Add attribute to fix B/C issue in list form field

Ruud68 commented 4 days ago

Pull Request for Issue #44484 .

Summary of Changes

with the merging of https://github.com/joomla/joomla-cms/pull/43842 the functionality that was present in Joomla for years has changed for extensions using that functionality. because there is no way to restore the used and required functionality this can be considered a B/C. Furthermore the PR introduced ambiguity as it changed the logic / functionality only for one field.

See discussion here: #44484

This PR adds an attribute with which a developer can 'restore' the showon functionality as it was, at least giving them the option to keep using the core ListField instead of developing a workaround.

this PR adds an form attribute: showonlocal="true/false" (default = false)

when setting showonlocal, the showon will acto on the local set value (like it has been for years in pré 5.2), when not set or when set to false, showon will act on the global value (like it is currently in 5.2)

pinging @LadySolveig

Testing Instructions

Set Global Configuration -> Contacts: Options -> Tab: Contact Information --> Show

Open a contact in edit mode or add a new contact.

Set Contacts: Edit -> Tab: Display -> Contact Information --> Use Global (Show)

open file ./administrator/component/com_contact/forms/contact.xml add attribute showonLocal="false" to field "show_info"

                <field
                    name="show_info"
                    type="list"
                    label="COM_CONTACT_FIELD_SHOW_INFO_LABEL"
                    class="form-select-color"
                    useglobal="true"
                    validate="options"
                    showonLocal="false"
                    >
                    <option value="0">JHIDE</option>
                    <option value="1">JSHOW</option>
                </field>

change showonLocal to "true"

in Contact Edit, Display tab, toggle Contact Information from "Use Global" to "Show"

Actual result BEFORE applying this Pull Request

Contact Edit , Display Tab Fields:

Name
Contact's Position

are always displaying

Expected result AFTER applying this Pull Request

Test Results 1

Contact Edit , Display Tab Fields:

Name
Contact's Position

are displaying

Test Results 2

Contact Edit , Display Tab Fields:

Name
Contact's Position

are hidden

Test Results 3

Contact Edit , Display Tab Fields:

Name
Contact's Position

are displaying

Link to documentations

Please select:

[ ] Documentation link for docs.joomla.org:
[ ] No documentation changes for docs.joomla.org needed
[ ] Pull Request link for manual.joomla.org:
[ ] No documentation changes for manual.joomla.org needed

HLeithner commented 4 days ago

can you please remove all unrelated changes (code style)

Ruud68 commented 4 days ago

can you please remove all unrelated changes (code style)

I see :( that means redoing the change outside my dev environment in a text editor. If this change is a viable candidate to get approved I will invest in this, for the draft it is now I do not want to invest anymore time in it other then discuss. Hope you understand :)

HLeithner commented 4 days ago

not sure why your dev environment restructure code automatically but ok, anyway you can use github editor todo this.

brianteeman commented 4 days ago

you seem to use a mix of showonlocal and showonLocal is this intentional?

HLeithner commented 4 days ago

you seem to use a mix of showonlocal and showonLocal is this intentional?

Hopefully I found all now

LadySolveig commented 4 days ago

I am not in favor for this. As I already mentioned in the issue, it actually only reveals structural misconceptions if a different value is to be used in the showOn than in the rest of the extension logic. However, I agree with you that it would have to be changed in the other fields as well and it is still handled inconsistently. 👍🏼

Ruud68 commented 4 days ago

you seem to use a mix of showonlocal and showonLocal is this intentional?

Hopefully I found all now

you found all 4 of them :)

Ruud68 commented 4 days ago

I am not in favor for this. As I already mentioned in the issue, it actually only reveals structural misconceptions if a different value is to be used in the showOn than in the rest of the extension logic. However, I agree with you that it would have to be changed in the other fields as well and it is still handled inconsistently. 👍🏼

and in this is exactly the issue: when a misconception is used for years, it is not a misconception anymore imo. developers depend on the functionality offered. This PR offers at least a choice to the developers (which in my book is a plus) and defaults to use the global value (as set by your PR): not breaking the default functionality of your PR.

Ruud68 commented 4 days ago

not sure why your dev environment restructure code automatically but ok, anyway you can use github editor todo this.

This is why, disabled formatting in the editor so should be okay now:

PHP code intelligence for Visual Studio Code.

Lossless PSR-12 compatible document/range formatting. Formats combined HTML/PHP/JS/CSS files too.

LadySolveig commented 4 days ago

I think it just continues to encourage developers to fall into the trap of creating dependencies that don't exist and continues to force misconceptions. I can understand that you don't want to rebuild your existing components. But I think if you want to keep this bug, it would have made more sense to have your own custom field that is derived from the ListField and that you can deliver with your extension and make available to other developers who have also fallen into the trap here.

But I think we can agree to disagree on that and that's fine too 🙂

Ruud68 commented 4 days ago

@LadySolveig this PR forces nobody, it adds the possibility (for which there are use cases) to use the local value instead of the global value. Nobody is forced, the developer has to take action if he wants to use the local value. So overall an improvement imo Who should be pinged here as well to chip in?

LadySolveig commented 4 days ago

I am not sure what you mean with 'chip in'. Your PR is available and can be tested. (Requires at least two human tests) You can also prepare the documentation for this change directly here. https://manual.joomla.org/ or wait if it gets merged. We are all Joomla - it is open source - so nobody there now 'to chip' 🙂

brianteeman commented 4 days ago

"I am not sure what you mean with 'chip in'.

chip in is english slang To chip in can mean to interrupt with a comment or remark, similar to chime in Example: We can fix up this old house if we all chip in and work together

bembelimen commented 3 days ago

I don't think that disabling the function via flag is the correct way forward, if this behaviour is needed (imho it's the wrong approach, but who is here to judge...), then we should improve the showOn JS adding more flexibility.

Ruud68 commented 3 days ago

hi @bembelimen , the 'flag' is added here: https://github.com/joomla/joomla-cms/blob/fab2ef624838b9d10cc09dac317d7d8b6db7304b/libraries/src/Form/Field/ListField.php#L196

and because that flag handled in showon.js in here: https://github.com/LadySolveig/joomla-cms/blob/52c6d325ae130ad6c54a6c585688d0c689bdcce9/build/media_source/system/js/showon.es6.js#L152-L157

This PR (other then reverting #43842 is the easiest fix. refactoring showon is not something I would take on as a fix: that would be something for a major upgrade.

And I mean fix as #43842 adds a feature but by doing so it breaks the existing functionality of using the local value (as it is being used by all other form fields).

Ruud68 commented 11 hours ago

I have just added the test instructions for this PR: https://github.com/joomla/joomla-cms/pull/44495#issue-2678598138

Happy testing :)

joomla / joomla-cms