Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Fix: Saving menu item fails. Call to a member function validate() on bool. Field "show_associations" #23700

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@ReLater
Copy link
Contributor

ReLater commented Jan 28, 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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 28, 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.

@ReLater

This comment has been minimized.

Copy link
Contributor Author

ReLater commented Jan 28, 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.

@ghazal

This comment has been minimized.

Copy link
Contributor

ghazal commented Jan 28, 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.

@Hackwar

This comment has been minimized.

Copy link
Member

Hackwar commented Jan 28, 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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 28, 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

@ChristineWk

This comment has been minimized.

Copy link

ChristineWk commented Jan 28, 2019

I have tested this item successfully on 23842e3

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

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 29, 2019

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

@joomdonation

This comment has been minimized.

Copy link
Contributor

joomdonation commented Jan 29, 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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 29, 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.

@joomdonation

This comment has been minimized.

Copy link
Contributor

joomdonation commented Jan 29, 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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 29, 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.

@Bakual

This comment has been minimized.

Copy link
Contributor

Bakual commented Jan 29, 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.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Jan 29, 2019

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

@joomdonation

This comment has been minimized.

Copy link
Contributor

joomdonation commented Jan 29, 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.

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 30, 2019

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

@ReLater

This comment has been minimized.

Copy link
Contributor Author

ReLater commented Jan 30, 2019

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

@ReLater ReLater closed this Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.