Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image alt attributes are double-escaped #806

Closed
GarrettAlbright opened this issue Feb 7, 2022 · 3 comments · Fixed by #810
Closed

Image alt attributes are double-escaped #806

GarrettAlbright opened this issue Feb 7, 2022 · 3 comments · Fixed by #810
Labels
bug Something isn't working right

Comments

@GarrettAlbright
Copy link
Contributor

Version(s) affected

2.1.1

Description

This also affects 1.6.7.

Input:

![The Mass Gravel & Sand area.](/assets/guides/107/fo4_mass_gravel_and_sand_header.jpg)

Output:

<p><img src="/assets/guides/107/fo4_mass_gravel_and_sand_header.jpg" alt="The Mass Gravel &amp;amp; Sand area." /></p>

The left and right angle brackets are also double-escaped.

How to reproduce

Create an image with alt text that contains a character that would need to be escaped.

Possible solution

No response

Additional context

No response

Did this project help you today? Did it make you happy in any way?

<3

@GarrettAlbright
Copy link
Contributor Author

GarrettAlbright commented Feb 7, 2022

Was thinking of doing a PR, but the way alt text is currently done in the code is so weird that I presume there must be some reason for it which is not apparent.

When the Image node is created, a text node is generated as a child with the alt text.

    public function __construct(string $url, ?string $label = null, ?string $title = null)
    {
        parent::__construct($url);

        if ($label !== null && $label !== '') {
            $this->appendChild(new Text($label));
        }

        $this->title = $title;
    }

Then in ImageRenderer:

    public function render(Node $node, ChildNodeRendererInterface $childRenderer): \Stringable
    {
        // …

        $alt          = $childRenderer->renderNodes($node->children());
        $alt          = \preg_replace('/\<[^>]*alt="([^"]*)"[^>]*\>/', '$1', $alt);
        $attrs['alt'] = \preg_replace('/\<[^>]*\>/', '', $alt ?? '');

        // …

        return new HtmlElement('img', $attrs, '', true);
    }

So it seems the first escape happens when that child node is rendered and that escaped text is put into a "normal" attribute for a tag and gets escaped again when that's rendered. But why? Why not just treat it as a normal attribute the whole way? 🤨

@colinodell colinodell added the bug Something isn't working right label Feb 7, 2022
@colinodell
Copy link
Member

It looks like this code was originally implemented in 0.3.0, way before we automatically escaped all attributes in 0.19.0. I think I just forgot to remove that old preg_replace() code 🙃

1.6.x is no longer receiving updates, but if you'd like to create a PR targeting the 2.0 branch I'd be happy to merge that into 2.0, 2.1, and 2.2 :)

@colinodell
Copy link
Member

Your fix has been released in 2.0.3, 2.1.2, and 2.2.2! Thanks again for the help here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants