I would like to be able to configure that the include pattern has priority over the exclude pattern.
The module now assumes the opposite.

Anyone that could point me in the right direction to start on this? Or this is a really bad idea?

CommentFileSizeAuthor
#1 regex_logic-1206276-1.patch863 bytesmonotaga
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

monotaga’s picture

Title: Make prioiry of include and exclude pattern matching configurable » Make priority of include and exclude pattern matching configurable
Status: Active » Needs review
FileSize
863 bytes

weseze,

I also agree that the "inclusion" of certain links as external should trump those links "excluded" (and therefore considered internal). It strikes me that a user is less likely to desire an internal link to be considered external, so these special cases should be given priority in the logic. Furthermore, by listing "Exclude links matching the pattern" first in the UI, the impression given to the user is that the this regex will be evaluated first and then the "Include links matching the pattern" will be evaluated, so that a URL matching both regex would ultimately be considered external.

If there's disagreement on this issue, then rather than committing a patch like the one I propose (attached), we could easily add a radio group in the admin UI asking which regex should be given priority. Wouldn't be hard at all.

Thoughts?

quicksketch’s picture

I'd prefer to avoid adding an option and stick with the approach that seems to make the most sense. I'm fairly sure I didn't give as much thought to this as you have, considering I don't think I've ever used the inclusion/exclusion patterns at all, they were added by feature request. Let's just get some appropriate testing on this and we can add it to the next version.

monotaga’s picture

Great. Thanks, quicksketch!

Test this patch, folks!

quinns’s picture

I found I was able to work around this problem by installing the Menu Attributes module.

quinns’s picture

Status: Needs review » Active
pbfleetwood’s picture

The patch fixes the problem.

Looking at the code, there really isn't anything that can go wrong with it; so it really ought to go into the next commit. Anyone who uses lots of modules ends up with far too many patches to manage. I have several that are for really simple code changes like this one.

Thank you.

pbfleetwood’s picture

Status: Active » Needs review

@quinns, please don't change an issue that has a patch awaiting review form 'Needs Review' to 'Active'. People need to see that there is a patch pending. I think most people would prefer a committed fix over a workaround, which is not to say that workarounds aren't appreciated.

pbfleetwood’s picture

Status: Needs review » Reviewed & tested by the community

I should mention that I tested the code in the patch (I applied the code manually) with 7.x-1.12. It has been working flawlessly. We've upgraded to Drupal core v.7.14 and this module continues to work well. Please apply this code change to the 7.x branch of this module. Thank you.

weseze’s picture

Version: 6.x-1.x-dev » 7.x-1.9
Status: Reviewed & tested by the community » Needs review

Does the patch apply cleanly to the D7 version as well?

elachlan’s picture

Version: 7.x-1.9 » 7.x-1.12
Status: Needs review » Reviewed & tested by the community

Looks like its RTBC to me.

quicksketch’s picture

@elachlan: Feel free to commit to all versions. Thanks for your work on this module!

elachlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Git.

elachlan’s picture

Status: Fixed » Closed (fixed)
elachlan’s picture

Status: Closed (fixed) » Needs work

Regression in all versions.

elachlan’s picture

Status: Needs work » Closed (fixed)

Fixed in all branches. Committed to Git.