- Issue created by @Grevil
- @grevil opened merge request.
- Status changed to Needs review
over 1 year ago 12:19pm 23 August 2023 - 🇩🇪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 PostponedTo 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 moduleI 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.