Category custom fields marked as 'subform' act as if they are not by nikosdion · Pull Request #35547 · joomla/joomla-cms

nikosdion · 2021-09-13T11:11:06Z

Pull Request for Issue #35543 .

Summary of Changes

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:

I changed \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.
I changed PlgSystemFields::onContentPrepareForm to load the missing form data when you are saving a com_content category.

Testing Instructions

In the backend of your site go to Content, Fields.
From the dropdown select “Category” and click on New.
Create a new field with the following parameters:
- Title: Test
- Label: Test
- Required: Yes. THIS IS IMPORTANT
- Only Use In Subform: Yes. THIS IS IMPORTANT
- Filter: Safe HTML. THIS IS IMPORTANT
Click on Save & Close
Go to Content, Categories
Edit any category
Click on Save

Actual result BEFORE applying this Pull Request

You get an error that the Test field is required.

Expected result AFTER applying this Pull Request

No error; the category is saved correctly.

Documentation Changes Required

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...

Backwards compatibility notes

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.

Technical notes

This is a two part issue.

Part I. “Only Use In Subform” doesn't do anything

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.

Part II. `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?

Part I: “Only Use In Subform” doesn't do anything

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Close gh-35543

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories This should only apply on com_content categories, not third party categories.

administrator/components/com_fields/src/Helper/FieldsHelper.php

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Okay, it could actually apply for any category, not just com_content.

crystalenka · 2021-09-16T07:07:08Z

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.}

administrator/components/com_fields/src/Helper/FieldsHelper.php

Fedik · 2021-09-16T17:47:04Z

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.

Fedik · 2021-09-18T07:34:16Z

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.}

administrator/components/com_fields/src/Helper/FieldsHelper.php

Fedik · 2021-09-20T08:30:20Z

RTC
It had 2 tests, and last change was a comment correction

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35547.}

Fedik · 2021-09-20T08:31:27Z

@wilsonge still have to decide when where and whether merge it ;)

wilsonge · 2021-09-21T13:35:58Z

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.

nikosdion · 2021-09-21T15:01:13Z

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

wilsonge · 2021-09-23T21:41:19Z

No complaints imminent. Merged. Thanks!

nikosdion · 2021-09-24T05:29:00Z

Thank you!

…omla#35547) * Subform–only fields cause saving a category to fail. Part I: “Only Use In Subform” doesn't do anything * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Close joomlagh-35543 * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories This should only apply on com_content categories, not third party categories. * Invert the if–block order Co-authored-by: Richard Fath <richard67@users.noreply.github.com> * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Okay, it could actually apply for any category, not just com_content. * Docblock Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

richard67 · 2021-10-28T13:43:46Z

Is seems there has been something forgotten, see #35924 .

janschoenherr · 2021-11-18T16:49:26Z

Hi, can one of you guys please review this PR: #35924

@arnepluhar

* Add sensible inline help to Global Configuration * Button to show/hide inline help Example in backend com_config * Add back the error handling * Show how this can be done in the frontend, too * Update build/media_source/system/js/inlinehelp.es6.js Co-authored-by: Brian Teeman <brian@teeman.net> * Update build/media_source/system/js/inlinehelp.es6.js Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev> * Address inconsistency of control and description DIV id's The controls have IDs like `jform_example`. The description DIVs had IDs like `jform[example]-desc`. This is horribly inconsistent and a pain to work with. Converted the description DIV IDs to the more consistent form `jform_example-desc`. * Toggle the aria-describedby attribute to follow the visibility of the description * Use simpler English * Make this into a proper BUTTON element and end–align it * Update administrator/language/en-GB/joomla.ini Co-authored-by: Quy <quy@fluxbb.org> * Update language string * Update language/en-GB/com_config.ini Co-authored-by: Brian Teeman <brian@teeman.net> * Clarify SEF URLs * Apply suggestions from code review DocBlock updates Co-authored-by: Phil E. Taylor <phil@phil-taylor.com> * [4.x] Tinymce updates (#35605) See https://www.tiny.cloud/docs/changelog/ for details This includes 5.9.0, 5.9.1 and 5.9.2 * typo in filename (#35613) * Remove JHELP_ strings for com_config and move the ref_key to the config.xml of the respective component. (#35479) Co-authored-by: Thomas Hunziker <werbemails@bakual.ch> * [4.0] Fix modal data attributes (#35626) * Update main.php * Update Bootstrap.php * [4.0] Remove JHELP_ strings for com_categories (#35534) * Category custom fields marked as 'subform' act as if they are not (#35547) * Subform–only fields cause saving a category to fail. Part I: “Only Use In Subform” doesn't do anything * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Close gh-35543 * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories This should only apply on com_content categories, not third party categories. * Invert the if–block order Co-authored-by: Richard Fath <richard67@users.noreply.github.com> * Subform–only fields cause saving a category to fail. Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Okay, it could actually apply for any category, not just com_content. * Docblock Co-authored-by: Richard Fath <richard67@users.noreply.github.com> * [4.0] Replace JHELP_ String with hardcoded ref-key (#35429) * Replace JHELP_ String with hardcoded ref-key * Simplifying helpTOC.php so it doesn't need to reverse lookup the JHELP_ strings. * Remove loading of joomla.ini and messing with language class property * alpha-sort help strings * Next batch * Last batch * Ooops Co-authored-by: Thomas Hunziker <werbemails@bakual.ch> * [4][CloudFlare Issues] Remove validation of the session by IP address (#35509) > IMHO the commend of @arnepluhar in #35509 should also count as a successful test. He did it as a GitHub code review. The rules about what constitutes a valid test should expand to GitHub code reviews; these didn't even exist when the rules were decided upon back in the day and they make FAR MORE SENSE than comments filed on a PR! Github is configured to allow maintainers merge only if all tests are successful. Thanks all. * Language Keys may have a : in it. (#35659) * Button to show/hide inline help Example in backend com_config * Whoops, wrong merge option * Lang string change * Apply language string suggestion * Remove empty line on language file, * Add sensible inline help to Global Configuration * Button to show/hide inline help Example in backend com_config * Add back the error handling * Show how this can be done in the frontend, too * Update build/media_source/system/js/inlinehelp.es6.js Co-authored-by: Brian Teeman <brian@teeman.net> * Update build/media_source/system/js/inlinehelp.es6.js Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev> * Address inconsistency of control and description DIV id's The controls have IDs like `jform_example`. The description DIVs had IDs like `jform[example]-desc`. This is horribly inconsistent and a pain to work with. Converted the description DIV IDs to the more consistent form `jform_example-desc`. * Toggle the aria-describedby attribute to follow the visibility of the description * Use simpler English * Make this into a proper BUTTON element and end–align it * Update administrator/language/en-GB/joomla.ini Co-authored-by: Quy <quy@fluxbb.org> * Update language string * Update language/en-GB/com_config.ini Co-authored-by: Brian Teeman <brian@teeman.net> * Clarify SEF URLs * Apply suggestions from code review DocBlock updates Co-authored-by: Phil E. Taylor <phil@phil-taylor.com> * Button to show/hide inline help Example in backend com_config * Whoops, wrong merge option * Lang string change * Apply language string suggestion * Remove empty line on language file, Co-authored-by: Brian Teeman <brian@teeman.net> Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev> Co-authored-by: Quy <quy@fluxbb.org> Co-authored-by: Phil E. Taylor <phil@phil-taylor.com> Co-authored-by: Stefan Wendhausen <stefan.wendhausen@tec-promotion.de> Co-authored-by: Thomas Hunziker <github@bakual.ch> Co-authored-by: Thomas Hunziker <werbemails@bakual.ch> Co-authored-by: Richard Fath <richard67@users.noreply.github.com> Co-authored-by: Benjamin Trenkle <bembelimen@users.noreply.github.com>

AndySDH · 2022-02-26T12:31:01Z

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)

Nicholas K. Dionysopoulos added 3 commits September 13, 2021 13:46

Subform–only fields cause saving a category to fail.

dcac224

Part I: “Only Use In Subform” doesn't do anything

Subform–only fields cause saving a category to fail.

5b57923

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Close gh-35543

Subform–only fields cause saving a category to fail.

e64e8ce

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories This should only apply on com_content categories, not third party categories.

joomla-cms-bot added the PR-4.0-dev label Sep 13, 2021

richard67 reviewed Sep 13, 2021

View reviewed changes

administrator/components/com_fields/src/Helper/FieldsHelper.php Outdated Show resolved Hide resolved

Nicholas K. Dionysopoulos and others added 2 commits September 13, 2021 16:47

Invert the if–block order

f5a007e

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

Subform–only fields cause saving a category to fail.

5879d56

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories Okay, it could actually apply for any category, not just com_content.

Fedik reviewed Sep 16, 2021

View reviewed changes

administrator/components/com_fields/src/Helper/FieldsHelper.php Show resolved Hide resolved

Fedik reviewed Sep 18, 2021

View reviewed changes

administrator/components/com_fields/src/Helper/FieldsHelper.php Outdated Show resolved Hide resolved

Docblock

6547e67

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 20, 2021

Fedik added the RMDQ ReleaseManagerDecisionQueue label Sep 20, 2021

wilsonge merged commit ff213f6 into joomla:4.0-dev Sep 23, 2021

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 23, 2021

wilsonge added this to the Joomla 4.0.4 milestone Sep 23, 2021

Quy removed the RMDQ ReleaseManagerDecisionQueue label Nov 18, 2021

richard67 mentioned this pull request Oct 6, 2022

Contact custom repeatable field migration J3 -> J4 #38909

Closed

Uh oh!

Conversation

nikosdion commented Sep 13, 2021

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Backwards compatibility notes

Technical notes

Part I. “Only Use In Subform” doesn't do anything

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories (when saving)

Uh oh!

Uh oh!

crystalenka commented Sep 16, 2021

Uh oh!

Uh oh!

Fedik commented Sep 16, 2021

Uh oh!

Fedik commented Sep 18, 2021

Uh oh!

Uh oh!

Fedik commented Sep 20, 2021

Uh oh!

Fedik commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonge commented Sep 21, 2021

Uh oh!

nikosdion commented Sep 21, 2021

Uh oh!

wilsonge commented Sep 23, 2021

Uh oh!

nikosdion commented Sep 24, 2021

Uh oh!

richard67 commented Oct 28, 2021

Uh oh!

janschoenherr commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndySDH commented Feb 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Part II. `PlgSystemFields::onContentPrepareForm` doesn't actually work for com_categories (when saving)

Fedik commented Sep 20, 2021 •

edited

Loading

janschoenherr commented Nov 18, 2021 •

edited

Loading