Skip to content

Fix #78003: strip_tags output change since PHP 7.3 #4153

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.

Sign up for GitHub

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

Jump to bottom
Closed
cmb69 wants to merge 2 commits into from
Closed

Fix #78003: strip_tags output change since PHP 7.3 #4153

cmb69 wants to merge 2 commits into from
+24 −0

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented May 12, 2019

A refactoring of the strip tags state machine[1] missed the special
treatment of depth > 0 when a > is encountered in state 3. We
re-add it for BC reasons.

[1] http://git.php.net/?p=php-src.git;a=commit;h=5cf64742773ddbf9af69d962a4d12b567fcf0084

@cmb69
Fix #78003: strip_tags output change since PHP 7.3
0560a4a 
A refactoring of the strip tags state machine[1] missed the special
treatment of `depth > 0` when a `>` is encountered in state 3.  We
re-add it for BC reasons.

[1] <http://git.php.net/?p=php-src.git;a=commit;h=5cf64742773ddbf9af69d962a4d12b567fcf0084>
@cmb69
Copy link
Member Author

cmb69 commented May 12, 2019

While for the given test case the old (i.e PHP-7.2 and PHP-7.3 with this patch) behavior is clearly more correct, for some other case the new (i.e. PHP-7.3 without this patch) behavior seems somewhat more reasonable. However, neither is actually optimal. Part of the problem is that we're still assuming that scripts are embedded inside of HTML/XML comments or CDATA sections, which may have been a reasonable assumption many years ago, but certainly isn't anymore. E.g. for <script><![CDATA[ if (i<j.length); ]]></script>text the contents of the script and particularly the < are ignored, while for <script>if (i<j.length);</script>text they are not. On the other hand, that shouldn't be an actual problem for the intended use of strip_tags().

@petk petk added the Bug label May 12, 2019
@nikic
Copy link
Member

nikic commented May 13, 2019

Do we need this in state_2 as well maybe?

@cmb69
Copy link
Member Author

cmb69 commented May 13, 2019

@nikic Ah, indeed. Thanks!

@cmb69
Copy link
Member Author

cmb69 commented May 13, 2019

Applied as 69bab6e. Thanks!

@cmb69 cmb69 closed this May 13, 2019
@cmb69 cmb69 deleted the bug-78003 branch May 13, 2019 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cmb69 @nikic @petk