The extlink module places an extra icon inside external links which break the look of the rrssb icons. There is a chance to configure extlink such that it ignores rrssb links and I suggest that this module actualy ensures that this is being set without waiting for the user doing that as they probably don't even know how to actually do that.

CommentFileSizeAuthor
#7 interdiff.2920800.4-7.txt1.85 KBjurgenhaas
#7 2920800-7.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch379 bytesjurgenhaas
#4 interdiff.2920800.2-4.txt1000 bytesjurgenhaas
#4 2920800-4.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch1.7 KBjurgenhaas
#2 2920800-2.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch1.06 KBjurgenhaas

Comments

jurgenhaas created an issue. See original summary.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Active » Needs review
StatusFileSize
new 2920800-2.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch1.06 KB

The patch covers both scenarios:

  • If rrssb is installed first, then rrssb_modules_installed recognises if extlink gets installed.
  • If extlink is installed first, then rrssb_install will take care of the situation.
adamps’s picture

adamps commented
Status: Needs review » Needs work

As a maintainer it is difficult to have code "if XX module is installed...". There are so many other modules and it would be time-consuming for me to test it in future.

As it stands, the code does not seem safe to work in the long term as it is reading another modules config and the dependency on specific ordering of events is a little tricky (e.g. the patch is missing code to undo the change).

I feel that the better fix is if you could raise the issue with extlink to add a scheme for mappings of "other modules" => "exclude CSS classes".

I guess as a fallback I would consider code here following a safer pattern that would never be called if extlink was missing, e.g. if extlink were to add a hook "hook_extlink_class_ignore".

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review
StatusFileSize
new 2920800-4.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch1.7 KB
new interdiff.2920800.2-4.txt1000 bytes

As a maintainer it is difficult to have code "if XX module is installed...". There are so many other modules and it would be time-consuming for me to test it in future.

Perfectly understandable. It's not a must, just a service that rrssb could offer as a convenience to its users. And as extlink is fairly popular with 60+ thousand reported installs I thought it might be worth the effort.

As it stands, the code does not seem safe to work in the long term as it is reading another modules config and the dependency on specific ordering of events is a little tricky (e.g. the patch is missing code to undo the change).

Agreed, but if there was an API from the other module, it would be the same effect to having an eye on it for future changes. Undoing the changes is now supported in the modified patch.

I guess as a fallback I would consider code here following a safer pattern that would never be called if extlink was missing.

Valid point, fixed in the modified patch. It only acts during install or uninstall of any module anyway, so not during regular operation of the site.

Don't get me wrong, don't want to push any feature on your module, but at least I wanted to improve the patch for the feature request such that it only acts if extlink is available and also reverts the changes when rrssb got uninstalled.

adamps’s picture

adamps commented
Status: Needs review » Postponed

OK so I'll set this as postponed. It's good to have your patch linked here in case it is useful to others.

If you want, you could raise a feature request with extlink to ask for an API to make it easier for other modules to integrate.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Thanks @AdamPS it's exactly my thinking too: let's have the patch available here for folks who were interested. And I also submitted a feature request to Extlink, see #2921077: Introduce an API to alter settings. Let's see what's happening.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Postponed » Needs review
StatusFileSize
new 2920800-7.rrssb_.Make-sure-it-works-nice-with-extlink-module.patch379 bytes
new interdiff.2920800.4-7.txt1.85 KB

Now that the API is available in extlink I have implemented that alter hook here and the solution is now really very simple.

adamps’s picture

adamps commented
Status: Needs review » Needs work

Something seems wrong - you hook is given an array and returns a string.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review

No, it IS given a string as well. That string contains a comma separated list of selectors and with the implode I'm just appending another selector. This simple "trick" works with an empty input as well as with a string that already contains values.

  • jurgenhaas authored cc42a6a on 8.x-2.x 
    Issue #2920800 by jurgenhaas: Make sure it works nice with extlink...
adamps’s picture

adamps commented
Status: Needs review » Fixed

Ah yes I see, cunning. I added a comment.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

phily’s picture

phily
he/him
Primary language French
Location 🇪🇺🇫🇷 Paris, France
commented

No patch needed, just add .rrssb-buttons in the CSS Matching exclude list within External Links settings.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Well, that a lazy approach and provides a bad UX. Why should the user be doing this manually if the module can do it because it knows exactly that this is requird? Or do you know of a scenario where both modules are enabled together and where this addition were not required?