? Pending

User tests: Successful: Unsuccessful:

avatar crystalenka
crystalenka
21 Nov 2022

Summary of Changes

According to MDN, the rel attribute on <link> and <a> elements can have multiple space-separated values.

This PR makes this a little easier by turning the rel field into a fancy select list and allows multiple values, which get imploded into a string before getting passed back to the layout (so there shouldn't be B/C problems).

Testing Instructions

For this PR to be a success a few things must be true:

  1. Existing rel fields must be converted over without loss of data.
  2. You must be able to add multiple link type values and have them show up on the link in the front end.

To test:

  1. BEFORE applying the patch, make a menu item of type URL and go to the "Link Type" tab. Select a link type from the "link rel attribute" dropdown.
  2. Go to the front end and make sure the link is displaying correctly, with the selected attribute getting rendered in the HTML. (It should look something like <a href="url here" rel="selected-attribute">.)
  3. Apply the patch.
  4. Success criteria 1: Go to the front end and make sure the link is still displaying the same way as before.
  5. Go back to the link type settings and select multiple attributes. Save.
  6. Success criteria 2: Go to the front end and make sure the selected attributes display in the html, separated by spaces. (It should look something like <a href="url here" rel="selected-attribute another-attr attribute-three">.)

Actual result BEFORE applying this Pull Request

Could not select multiple rel attributes.

Expected result AFTER applying this Pull Request

Existing links are unchanged; new capability of selecting multiple rel attributes works well.

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

I don't know yet whether documentation is needed.

avatar crystalenka crystalenka - open - 21 Nov 2022
avatar crystalenka crystalenka - change - 21 Nov 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Nov 2022
Category Administration com_menus Modules Front End
avatar crystalenka crystalenka - change - 21 Nov 2022
Labels Added: ?
avatar brianteeman
brianteeman - comment - 21 Nov 2022

Must admit to never having seen this before. But following your links for adding missing values I see other values we have in our list that are not valid

actually its just prefetch that isdwrong. The others that I thought were invalid are non standard values created by google and should be included

avatar crystalenka
crystalenka - comment - 21 Nov 2022

Must admit to never having seen this before. But following your links for adding missing values I see other values we have in our list that are not valid

actually its just prefetch that isdwrong. The others that I thought were invalid are non standard values created by google and should be included

Yes, I saw prefetch too (which doesn't actually do anything when it's on an <a> tag) but not sure if we can straight up remove it without breaking b/c.

avatar SharkyKZ
SharkyKZ - comment - 21 Nov 2022

Rebase on 5.0.

avatar crystalenka
crystalenka - comment - 21 Nov 2022

Rebase on 5.0.

Can I ask why? It's not a massive change or backwards compatibility break unless I've missed something. Really very minor change, there are bigger ones going into 4.3.

avatar SharkyKZ
SharkyKZ - comment - 22 Nov 2022

Rebase on 5.0.

Can I ask why? It's not a massive change or backwards compatibility break unless I've missed something. Really very minor change, there are bigger ones going into 4.3.

You missed the part where you had to make amends in the login module for it not to crash. Extension developers must not be required to make changes in patch and minor versions. But you're right in that many more breaking changes have been introduced lately. It's almost as if the project does not follow SemVer anymore and screwing over Joomla users is the norm. So it's fine, another B/C break that bricks sites in a patch release is fine ?.

avatar crystalenka
crystalenka - comment - 22 Nov 2022

Snark is unnecessary, it was a genuine question. Please be kind.

I didn't make any changes to the login module.

I made a very minor change to the menu module helper, which still works with data saved prior to this change. (Hence my test case 1.)

Probably I could do it more elegantly as evidenced by the review comments, but I genuinely don't see how this is a breaking change when there is a check for the data type and it can handle both ways.

avatar crystalenka crystalenka - change - 22 Nov 2022
The description was changed
avatar crystalenka crystalenka - edited - 22 Nov 2022
avatar crystalenka
crystalenka - comment - 22 Nov 2022

Okay, so, I understand better now how the changes I suggested are a b/c break.

Is there a way to approach this that would allow multiple rel values without the b/c break? Earlier in the process maybe, so that it gets turned to a string before any 3rd party component or plugin or template would call them?

I thought the menu helper would be early enough, but I understand some people may bypass it.

avatar ReLater
ReLater - comment - 22 Nov 2022

Copied from #39275 on request

ReLater:
In terms of maintenance and usability in other extensions, wouldn't it be better to introduce a new form field in Joomla? RelAttributesField.php or something?

crystalenka:
And do what with the input/data/current field? Wouldn't that be a bigger b/c break?

ReLater:
the idea is instead of using

<field
 name="menu-anchor_rel"
 type="list"
 label="COM_MENUS_ITEM_FIELD_ANCHOR_REL_LABEL"
 default=""
 validate="options"
>

to use

<field
 name="menu-anchor_rel"
 type="RelAttributes"
 label="COM_MENUS_ITEM_FIELD_ANCHOR_REL_LABEL"
 default=""
 validate="options"/>

You would have a single location to change/add/(remove) new options.

RelAttributesField.php could be a form field that extends ListField and thus would inherit all attributes of list field and behavior (multiple, layout etc.).

Concerning B\C break: I have absolutely no idea if that is one. The idea with a new field.

Was just an idea because I saw that you was correcting the options in 2 files.

avatar HLeithner
HLeithner - comment - 2 May 2023

This pull request has been automatically rebased to 5.0-dev.

avatar HLeithner
HLeithner - comment - 30 Sep 2023

This pull request has been automatically rebased to 5.1-dev.

avatar HLeithner
HLeithner - comment - 24 Apr 2024

This pull request has been automatically rebased to 5.2-dev.

avatar HLeithner HLeithner - change - 24 Apr 2024
Title
Allowing menu items to have multiple 'rel' values
[5.2] Allowing menu items to have multiple 'rel' values
avatar HLeithner HLeithner - edited - 24 Apr 2024
avatar HLeithner
HLeithner - comment - 2 Sep 2024

This pull request has been automatically rebased to 5.3-dev.

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] Allowing menu items to have multiple 'rel' values
[5.3] Allowing menu items to have multiple 'rel' values
avatar HLeithner HLeithner - edited - 2 Sep 2024

Add a Comment

Login with GitHub to post a comment