Split Icon links in their own meta tag definition

Created on 23 August 2023, over 1 year ago
Updated 7 September 2023, about 1 year ago

Problem/Motivation

Currently, all the favicon icon links are put in head through a single Drupal meta tag element (through the prefix, which also seems incorrect).

This makes it hard for other modules to remove single tags through 'hook_page_attachments_alter()'.

Steps to reproduce

Proposed resolution

Split Icon links in their own meta tag definition.

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Needs review

Version

1.0

Component

Code

Created by

🇩🇪Germany Grevil

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Grevil
  • 🇩🇪Germany Grevil

    Can't create a dedicated branch. Working on 1.x.

  • @grevil opened merge request.
  • Status changed to Needs review over 1 year ago
  • 🇩🇪Germany Grevil

    Alright, all done, please review!

    I do not completely like the identification of each tag using:

    'responsive_favicons_tag_' . $key
    

    yet.
    First, I used the rel and name value, to identify the tag, but this can obviously be duplicate.

    The used preg_match should handle most 'name' and 'rel' cases. Alternatively, we could also parse the HTML strings to DOMElements first and get the values from the resulting DOMElement. This isn't easily done, as we can not simply create the DOMElement out of an HTML string. Instead we would need something like this, which would create too much overhead in my book.

  • 🇩🇪Germany Anybody Porta Westfalica

    Thanks @Grevil for the implementation and explanation. Indeed, this feels a bit dangerous and unsafe but I don't have a better idea yet, to be honest.

    I think we need to discuss the various options with @thomas.frobieter. Especially the combination with PWA.

    For the records: One alternative idea could be to paste the code from https://realfavicongenerator.net/ into the textarea and split it into separate fields, then using JS.

    But for now I think the most propable option is to just leave everything as-is and just tell the user to remove duplicate values from the text, which are already provided by PWA in the PWA warning message here:
    Let pwa overwrite "responsive_favicons"'s manifest and theme-color, if both modules are activated Postponed

    To detect if there are conflicting values, we could just search the responsive_favicons config value for certain texts like:

    • rel="manifest"
    • name="theme-color"
    • ...?

    and only show the warning, if they are present, otherwise show a green message with information and tell the user he seems to already have solved the conflict.

    What do you think?

    If you agree, let's copy this comment over to the PWA task and decide if we should close this or have any better idea.

  • But for now I think the most propable option is to just leave everything as-is and just tell the user to remove duplicate values from the text, which are already provided by PWA in the PWA warning message here:
    #3360610: Warn about possible conflict with responsive_favicons module

    I think a warning is enough. People who work with the PWA module are not novice users, they should be able to handle this sort of thing.

  • 🇩🇪Germany Grevil

    To detect if there are conflicting values, we could just search the responsive_favicons config value for certain texts like:

    rel="manifest"
    name="theme-color"
    ...?
    and only show the warning, if they are present, otherwise show a green message with information and tell the user he seems to already have solved the conflict.

    I agree, but I'd say we keep this issue open, as the MR's approach feels better than putting everything in a meta tag defintion "prefix".

    Let's see, what the maintainer has to say about this.

Production build 0.71.5 2024