User tests: Successful: Unsuccessful:
Pull Request for Issue #35543 .
Category custom fields marked as “Use Only In Subform” were loaded as regular fields in all categories. This is a two–part bug (see Technical Notes at the bottom).
As a result I made the following changes:
\Joomla\Component\Fields\Administrator\Helper\FieldsHelper::getFields
to only load non–subform–only fields by default with a new option to return all fields, including the subform–only, and made the necessary changes to SubfieldsField and PlgFieldsSubform.You get an error that the Test field is required.
No error; the category is saved correctly.
None. I mean, it would be nice if someone documented the “Only Use In Subform” field as now there's no tooltip and no documentation. What does this do? Nobody knows. Even looking at the code it was not obvious, presumably because it did nothing at all to begin with...
The \Joomla\Component\Fields\Administrator\Helper\FieldsHelper::getFields
method has changed so that, by default, it does not return ALL fields but only those not marked as “Only Use In Subform”. Is this a b/c break? I would say no, because the intent of this option was to make these fields unavailable anywhere EXCEPT the subform custom field. Fixing a bug so that something works as it should, per the meager code documentation, shouldn't be considered a b/c break.
If it is to be considered a b/c break then the “Only Use In Subform“ feature should be removed from 4.0 as it cannot be fixed otherwise (well, just part II of the fix will kinda–sorta fix it but it's an ancillary artefact at best). Then again, removing a feature — even one that never worked — would be a b/c break.
So I am going to ping @wilsonge and @zero-24 and let them sort out the decision–making process of what constitutes a backward compatibility break in this context.
This is a two part issue.
The new “Only Use In Subform” option in a Custom Field's definition not doing what it's supposed to do.
Supposedly, custom fields with this option enabled would be excluded from everything except defining or rendering a subform custom field.
In reality, this option does nothing useful. It displays a badge in the fields list. Oh, yeah, it also makes the Custom Fields where this option is enabled have a category list of [-1]
. That's the beginning, middle and end of it. Totally useless.
I changed \Joomla\Component\Fields\Administrator\Helper\FieldsHelper::getFields
to only load non–subform–only fields by default. If you want to get all fields, including subform–only, you need to pass true
to a new fifth parameter. Note that the ONLY use case for retrieving all fields, including subform–only, is the SubfieldsField and the subform field itself. Third–party software SHOULD NOT receive those fields outside of the context of a subform custom field, therefore this is not a b/c break.
PlgSystemFields::onContentPrepareForm
doesn't actually work for com_categories (when saving)You will see that the system plugin ALLEGEDLY has special handling for editing categories in com_categories. Allegedly, because it works on the edit page but NOT when saving! When saving all fields from all categories are loaded since the plugin runs before the form data is loaded from the request, meaning there is no category filter present.
You see, $data
is an empty array when you the onContentPrepareForm
event is fired when saving the form. I see that the choice to not populate the form data when loading the form in \Joomla\CMS\MVC\Controller\FormController::save
dates back to 2010 when Andrew made a commit about fixing some unspecified issue saving plugins.
The problem is that for com_categories and only this component we actually need to load the form data from the request when saving data. Otherwise PlgSystemFields:: onContentPrepareForm sees that $data is empty and doesn't set the category ID. As a result FieldsHelper::prepareForm
populates $assignedCatids
to null, therefore it loads all fields from all categories. If any of these fields is required AND has a validator which doesn't accept NULL values — like the ones @crystalenka mentioned — you get a save error.
There are a few options to fix it. One is to override CategoryController::save. However, this results in severe code duplication which will be a pain in the posterior to manage whenever the base FormController::save method is updated.
The other option is to force CategoryModel::getForm to always load the data. However, this may break b/c; I'm especially concerned about third party plugins which may use the empty $data as a signal when figuring out which fields to add to the category edit view.
The final option is to change PlgSystemFields::onContentPrepareForm itself. Since we're doing special handling for com_categories we might just as well populate data
from the request if we are saving the form. This is what I plan on doing. To keep things b/c this nasty workaround will ONLY apply to com_content categories, not third party extensions' categories. In fact, custom fields SHOULD only apply to com_content categories — which is not the case now — but that's a different bug for a different day, n'est ce pas?
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_fields Front End Plugins |
Labels |
Added:
?
|
Looks good to me.
The filter.only_use_in_subform
was add in #33096 ,
@joomdonation can you please have a look, I think you forgot to add the set('filter.only_use_in_subform')
for it or?
The Article does not have this bug because the fields always filtered by filter.assigned_cat_ids
, however for category this filter always empty.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
It had 2 tests, and last change was a comment correction
Joyfully since PHP 8 this counts as a b/c break because method signatures aren't allowed to change if people are subclassing example: https://3v4l.org/iedIW . This is also causing issues with other PRs such as #35476 (where I stole the example link from).
I think it's fairly unlikely that people are subclassing fieldshelper to be honest so my gut is to merge this. However I'm going to leave this here a few days to see if anyone else has any strong objections or feelings on the matter.
I'd be surprised if anyone subclasses that. The way it's constructed it's just a magic static helper you can't really modify without hacking core — or making a PR
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-09-23 21:41:09 |
Closed_By | ⇒ | wilsonge | |
Labels |
Added:
?
?
|
No complaints imminent. Merged. Thanks!
Thank you!
Thank you @nikosdion for making this, a great fix! This also fixed the same issue for the Users component! (as they have no category either, so they ran into the same problem)
I have tested this item✅ successfully on 5879d56
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35547.