? Maintainers Checked Pending

User tests: Successful: Unsuccessful:

avatar roland-d
roland-d
14 Aug 2022

Pull Request for Issue #38244

Summary of Changes

This PR fixes a few issues with the menu item field:

  • Cast value to integer as menu item IDs are always numbers
  • Check if the menu item exists in case the number is wrong or unpublished
  • Use single quotes around strings
  • Use HTMLHelper for rendering link

Testing Instructions

  1. Create a custom field of the type List Of Menu Items (menuitem)
  2. Save the custom field
  3. Create an article and select a menu item on the Fields tab
  4. Create a menu item of the type Single Article and select the new created article
  5. Visit the article on the front-end
  6. You should see the menu item listed
  7. Unpublish the menu item that was selected in the article
  8. Refresh the page
  9. You see a warning Attempted to read property title on null
  10. Apply patch
  11. Refresh the page
  12. The link and warning should be gone

Actual result BEFORE applying this Pull Request

A warning Attempted to read property title on null when menu item is not available

Expected result AFTER applying this Pull Request

No warning is displayed and no link is displayed

Documentation Changes Required

None

avatar roland-d roland-d - open - 14 Aug 2022
avatar roland-d roland-d - change - 14 Aug 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Aug 2022
Category Front End Plugins
avatar chmst
chmst - comment - 14 Aug 2022

I have tested this item successfully on a066d43


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

avatar chmst chmst - test_item - 14 Aug 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 14 Aug 2022

For my education can you please explain the changes regarding use statements.

Previously I was told that if its used only once in the file then it should be
class PlgFieldsMenuitem extends \Joomla\Component\Fields\Administrator\Plugin\FieldsPlugin

and not

use Joomla\Component\Fields\Administrator\Plugin\FieldsPlugin;
class PlgFieldsMenuitem extends FieldsPlugin

But you have changed this in the file

avatar superknutsel
superknutsel - comment - 14 Aug 2022

I have tested this item successfully on a066d43


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

avatar superknutsel superknutsel - test_item - 14 Aug 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 14 Aug 2022

Sorry but this is not the correct fix.

The custom field should work the same as the standard fields see https://docs.joomla.org/Menuitem_form_field_type

As you can see I missed adding the published option and just used the default

published (optional) determines whether all menu items are listed or only published menu items. If state is '0' then all menu items will be listed. If state is '1' then only published menu items will be listed. You also can use comma separated values like '1,2'.

Removing the ability to select an unpublished menu item severely limits the usability of this field ie you can not create links in advance of publishing the menu item

avatar brianteeman
brianteeman - comment - 14 Aug 2022

I have tested this item ? unsuccessfully on a066d43


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

avatar brianteeman brianteeman - test_item - 14 Aug 2022 - Tested unsuccessfully
avatar roland-d
roland-d - comment - 14 Aug 2022

For my education can you please explain the changes regarding use statements.

For me you always add namespaces to the list of use statements, so you have a complete overview of the namespaces you include.

Previously I was told that if its used only once in the file then it should be

I am not aware of such rule nor do I understand why it should be that way. The only time you could use the full path is when you have a naming conflict but even then namespaces can be aliased. If it is a rule, then it should be reverted.

Sorry but this is not the correct fix.

Fine, I am going to revert the custom field and we can apply proper fixes and get it ready for 4.3.

avatar brianteeman
brianteeman - comment - 14 Aug 2022

There are over 50 instances of
class xxxxxx extends \JoomlaXXXXXXXXXXXXXX

in fact almost all of those are for field plugins just like this one

avatar brianteeman
brianteeman - comment - 14 Aug 2022

but if you want to change that and always use namespaces then that is fine by me but then dont you also need to remove
* @phpcs:disable PSR1.Classes.ClassDeclaration.MissingNamespace

avatar roland-d roland-d - change - 14 Aug 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-08-14 11:32:07
Closed_By roland-d
Labels Added: ? Maintainers Checked
avatar roland-d roland-d - close - 14 Aug 2022
avatar brianteeman
brianteeman - comment - 14 Aug 2022

The problem must be in the parent groupedlistfield and not here as if you are using the usergroup list field then the exact same testing instructions do not produce the error

avatar brianteeman
brianteeman - comment - 14 Aug 2022

actually I misread part of this and you missed it in my comment

I read it as this pr removed the ability to SELECT an unpublished menu item which is what I said was desired.

Removing the ability to select an unpublished menu item severely limits the usability of this field ie you can not create links in advance of publishing the menu item

I see now that the check is only for the output of the link

Add a Comment

Login with GitHub to post a comment