Problem/Motivation

Sometimes the page already contains links manually added on terms.
When this happens and the $first_only option is used, glossify will still add a link on the next occurrence of the term.
We would prefer that it considers the term already linked.

Steps to reproduce

<a href="http://example.com/">Lorem</a> ipsum.
Lorem ipsum.

If one of the terms is lorem, the second occurrence will be replaced although the first one is already a link.

Proposed resolution

Before parsing the text, GlossifyBase::parseTooltipMatch() should look for links containing terms.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork glossify-3511457

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff opened merge request !44

prudloff’s picture

prudloff commented
Status: Active » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3102570: Glossify + Paragraphs: Multiple glossary links instead of one per node

#3102570: Glossify + Paragraphs: Multiple glossary links instead of one per node looks related?

grevil made their first commit to this issue’s fork.

grevil’s picture

grevil commented
Status: Needs review » Needs work

This fix currently only covers the case, when the glossify type is set to "links" or "tooltips_links". This doesn't cover the 'tooltips' case. I understand that this is a bit confusing, as the "First match only" description is a bit wrong:
Match and LINK only the first occurrence per field
This setting also effects the tooltip creation, not only the link creation.

EDIT: Ok, never mind, it already works with "abbr" tags, because they are not excluded by the xpath query, @anybody, why don't we exclude "abbr" tags?

grevil’s picture

grevil commented

And I think we should put the logic into the "$text_nodes" foreach (in the reinserting first match if case), IF POSSIBLE! Might not be possible, as we exclude anchor tags in general, but maybe we can pass a flag, letting the reinsertion if case know, whats the case here.

grevil’s picture

grevil commented
Related issues: -#3102570: Glossify + Paragraphs: Multiple glossary links instead of one per node

Issue is unrelated.

grevil’s picture

grevil commented

Ok, "abbr" tags should be handled properly now through #3519291: "abbr" tags are not being excluded from the xpath query.

Now my concern in #6 is valid again:

This fix currently only covers the case, when the glossify type is set to "links" or "tooltips_links". This doesn't cover the 'tooltips' case. I understand that this is a bit confusing, as the "First match only" description is a bit wrong:

Match and LINK only the first occurrence per field

This setting also effects the tooltip creation, not only the link creation.

joao.ramos.costa’s picture

joao.ramos.costa commented

#4 Seems related.
I think the current MR there would fix this, no ?
Regards.

grevil’s picture

grevil commented

This issue only targets glossified words inside links per textfield not properly working with the "first_only" setting.
#3102570: Glossify + Paragraphs: Multiple glossary links instead of one per node suggests a solution working across multiple textfields.

Update: Sorry, this explanation was not very insightful. The MR in #3102570: Glossify + Paragraphs: Multiple glossary links instead of one per node wouldn't fix this issue. The problem of this issue is, that glossified words wrapped inside a or abbr tags would not be considered as a first occurence (when first_only ist enabled).

This is because we completely ignore a and abbr tags in our processing, so we don't accidentally rewrap them again and therefore, they are excluded from the "first_only" logic inside our text processing logic.

grevil’s picture

grevil commented
Status: Needs work » Reviewed & tested by the community

Alright LGTM! I added abbr support and added tests. LGTM! Thank you!

grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

All green, merging.

  • grevil committed 4b45024e on 3.x authored by prudloff 
    Issue #3511457 by grevil, prudloff: Don't replace term if there is...
joao.ramos.costa’s picture

joao.ramos.costa commented

Hi @grevil, thank you :) !

Status: Fixed » Closed (fixed)

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

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Hmm I'm not sure about this change. We use the tooltip mode (we don't link to the tooltip term pages, we just use the tooltip abbreviations). With this change, if the term first appears in some link on the page, it doesn't get tooltip'ed anymore. I think it should. The link is not going to the term page in our case - it goes to some other page on the site. I think that's what #6 mentioned, but I'm not sure this was addressed? Maybe this new functionality should be limited to only configurations that use "links" or "tooltip_links" setups?