Skip to content

img: fix srcset pattern and src default value#9

Merged
Kdecherf merged 3 commits intofossar:masterfrom
Kdecherf:fix/img
Feb 1, 2022
Merged

img: fix srcset pattern and src default value#9
Kdecherf merged 3 commits intofossar:masterfrom
Kdecherf:fix/img

Conversation

@Kdecherf
Copy link

@Kdecherf Kdecherf commented Nov 20, 2021

The first change updates srcset pattern to support URL without any descriptor as it is supported by the standard, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Img#attr-srcset

The second change sets the default img src value to an empty gif. Without this change, HTMLawed adds src="src" to invalid tags which can lead to unexpected behaviors on browsers or http client (e.g. Graby).

Note: this PR is based on #8

htmLawed.php Outdated
if ('srcset' === $k) {
$v2 = '';
$pattern = "/(?:[^\"'\s]+\s*(?:\d+w|\d+(?:\.\d+)?x)+)/";
$pattern = "/(?:[^\"',\s]+(?:\s(?:\d+w|\d+(?:\.\d+)?x))?)/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this really needs test coverage. If I am reading https://html.spec.whatwg.org/multipage/images.html#srcset-attribute right, it should be \s+.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm indeed, I'll rework it and I'll try to add tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the regex in 51698de

Added tests for this case along Github Actions workflow in e012d63

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poke @j0k3r


// rqd attr
static $eAR = ['area' => ['alt' => 'area'], 'bdo' => ['dir' => 'ltr'], 'command' => ['label' => ''], 'form' => ['action' => ''], 'img' => ['src' => '', 'alt' => 'image'], 'map' => ['name' => ''], 'optgroup' => ['label' => ''], 'param' => ['name' => ''], 'style' => ['scoped' => ''], 'textarea' => ['rows' => '10', 'cols' => '50']];
static $eAR = ['area' => ['alt' => 'area'], 'bdo' => ['dir' => 'ltr'], 'command' => ['label' => ''], 'form' => ['action' => ''], 'img' => ['src' => 'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', 'alt' => 'image'], 'map' => ['name' => ''], 'optgroup' => ['label' => ''], 'param' => ['name' => ''], 'style' => ['scoped' => ''], 'textarea' => ['rows' => '10', 'cols' => '50']];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus it will prevent invalid value to be inserted when trying to fix img
tags.

Before this change the line:

  <img alt="Hello world">

led to:

  <img alt="Hello world" src="src">

Now, the src attribute will be initialized with the value:

  data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
@Kdecherf
Copy link
Author

@j0k3r @jtojnar tests on php 7.2 and php 7.3 are failing because of a dependency issue.

Knowing that php < 7.4 are not supported anymore, could be bump the minimum required php version to 7.4?

@Kdecherf Kdecherf requested a review from j0k3r January 22, 2022 18:19
Copy link

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should stay on v3.4.0 for friendsofphp/php-cs-fixer to avoid the 7.4 bump.
As it's a dev deps, I think it's preferable to keep 7.2 compats.
Maybe you should also lower PHPUnit?

with:
php-version: "${{ matrix.php }}"
coverage: none
tools: pecl, composer:v1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tools: pecl, composer:v1
tools: pecl, composer:v2

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to keep v1 or you forget it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good catch, just forgot it. Fixed.

@Kdecherf
Copy link
Author

Kdecherf commented Feb 1, 2022

@j0k3r I've updated the lock file to keep support of PHP 7.2 and 7.3.

Interesting thing to note: I was forced to call composer update with a PHP 7.2 install to have compatible dependencies.

@j0k3r
Copy link

j0k3r commented Feb 1, 2022

@Kdecherf
Copy link
Author

Kdecherf commented Feb 1, 2022

@Kdecherf https://getcomposer.org/doc/06-config.md#platform wink

@j0k3r not sure of the usage of the property, should I fix it before merging this PR or is it fine for now?

@j0k3r
Copy link

j0k3r commented Feb 1, 2022

I still prefer to define it so nobody will have error if they update the proches using a higher version of PHP.
Put the version 7.2.34

@j0k3r
Copy link

j0k3r commented Feb 1, 2022

Looks good to me!

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
@Kdecherf Kdecherf merged commit cc42f21 into fossar:master Feb 1, 2022
@Kdecherf Kdecherf deleted the fix/img branch February 1, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants