? Pending

User tests: Successful: Unsuccessful:

avatar ReLater
ReLater
28 Jan 2019

Pull Request for Issue #23696

Related and already merged fix: #23501

Testing Instructions

  • Install a J4 nightly build of today (Monday, 28 January 2019) or update an older J4 with that package

  • Go to menu items manager in back-end.

  • Open Home menu item (type "Articles > Featured Articles")

  • Save it.

  • Error like described in #23696 (comment)

  • Apply patch

  • Try again to save the menu item.

  • Should be successful.

avatar ReLater ReLater - open - 28 Jan 2019
avatar ReLater ReLater - change - 28 Jan 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Jan 2019
Category Administration com_menus
avatar infograf768
infograf768 - comment - 28 Jan 2019

I am concerned by one aspect:
If a 3pd uses the same field name for other stuff than articles, it would remove the field.

avatar ReLater
ReLater - comment - 28 Jan 2019

I'm also concerned because I don't understand this necessary hardcoding of field names inside models when I read the introduction of pr #12414 of @Hackwar concerning "hardcoded behavior" and "hardcoded filters" . At the moment this pr here and #23501 are just bad "outsourcing of occuring new issues" somehow.

On the other hand the line $form->removeField('show_associations', 'params'); provides the fields group, too. Shouldn't that be sufficient to avoid field name conflicts? I don't know.

I don't know how similiar behavior can be implemented in the concerning field classes directly or if that's possible anyhow.

avatar ghazal
ghazal - comment - 28 Jan 2019

I have tested this item successfully on 23842e3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23700.

avatar ghazal ghazal - test_item - 28 Jan 2019 - Tested successfully
avatar Hackwar
Hackwar - comment - 28 Jan 2019

@infograf768 you have effectively already hardcoded the usage of that parameter name. If you have issues with the Form changes, then maybe make sure that the associations field doesn't behave in the way it does.

avatar infograf768
infograf768 - comment - 28 Jan 2019

@Hackwar
I do not understand what you are saying.

After #12414 , @joomdonation had to do #23501

I had nothing to do with these changes.

It is not an association field per se as in the Associations Tab.
show_associations just lets show for an article in frontend an icon or lang tag of the associated article in the block.

It looks partly broken for now in 4.0 but this is what it looks like in 3.x

screen shot 2019-01-28 at 18 06 39

avatar ChristineWk
ChristineWk - comment - 28 Jan 2019

I have tested this item successfully on 23842e3

- After installing nightly build (update package) from 28th

avatar ChristineWk ChristineWk - test_item - 28 Jan 2019 - Tested successfully
avatar infograf768
infograf768 - comment - 29 Jan 2019

Please do NOT set this as RTC until @Hackwar and @joomdonation give more precise informations on a better way to solve this.

avatar joomdonation
joomdonation - comment - 29 Jan 2019

We have two options here:

  1. Check in all of our core components to see which one used that field and do the same fix like I did on this PR #23501

  2. Or restore the original code which @Hackwar implemented in his PR. Basically, the command below before this line https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Form.php#L1202

if (!$fieldObj instanceof FormField)
{
    continue;
}

To me, #1 would be better.

avatar infograf768
infograf768 - comment - 29 Jan 2019

Check in all of our core components to see which one used that field and do the same fix like I did on this PR #23501

As explained above (see screenshot) this is only used for com_content articles in the info_block/ associations layout.

avatar joomdonation
joomdonation - comment - 29 Jan 2019

I don't work much with multilingual, so I am unsure. I will try to look at this issue later today to see what's the actual reason of the error and hopefully can come up with a proper fix.

avatar infograf768
infograf768 - comment - 29 Jan 2019

Or restore the original code which @Hackwar implemented in his PR. Basically, the command below before this line https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Form.php#L1202

Tested this and it works fine.

avatar Bakual
Bakual - comment - 29 Jan 2019

It's certainly wrong to have this code in com_menus. It's a com_content specific parameter.
If you want this to be conditional, you will have to try with a custom formfield for com_content.

avatar SharkyKZ
SharkyKZ - comment - 29 Jan 2019

@joomdonation Unless this change is documented somewhere, option 2 is the proper fix. The form should not fail when FormField::setup() returns false.

avatar joomdonation
joomdonation - comment - 29 Jan 2019

@SharkyKZ I think the field should not belong to the form when association is disabled, so for me, remove the field when it's not needed is better than just ignoring the error.

avatar wilsonge
wilsonge - comment - 30 Jan 2019

I think this is a better fix for handling when setup returns false #23716 - see what you guys think

avatar ReLater
ReLater - comment - 30 Jan 2019

Closing in favour of #23716 that also works for articles when you revert patch #23501

avatar ReLater ReLater - close - 30 Jan 2019
avatar ReLater ReLater - change - 30 Jan 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-01-30 11:15:07
Closed_By ReLater
Labels Added: ?

Add a Comment

Login with GitHub to post a comment