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

Update HtmlReplacer.php #84

Closed
wants to merge 1 commit into from
Closed

Conversation

wilfriedwolf
Copy link
Contributor

Avoids attributes from different tags

Avoids attributes from different tags
@wilfriedwolf
Copy link
Contributor Author

Solves issue introduced by #83

@hostep
Copy link
Contributor

hostep commented Dec 11, 2024

I think you will also need to update the regex in unmaskAngleBracketsInsideQuotes, as it will also match things you don't want to change: https://regex101.com/r/KsPdmb/1

But in my opinion, this regex solution is a wrong approach to fix #82, it just will cause too many edge case issues as it works on the entire html output of a page. Can't we limit it to only operate on image/picture tags? So that it only applies to html output inside that foreach ($images as $image) { section?

@jissereitsma
Copy link
Contributor

Sorry for not chiming in more quickly. Indeed I overlooked things heavily while merging this PR. On top of this, the integration tests were failing (perhaps also because of other reasons) which is bad as well.

The current fix is still not good enough: What if there is somewhere a quoted part with > in it which should not be converted into >? In other words, @hostep is right that this change should not be temporarily changing strings in the entire HTML, but only in the image tag at hand.

I'm working on this.

@jissereitsma
Copy link
Contributor

I've now added an alternative fix, simply by adding a more complex regex in the right place: 867ef31#diff-56e9cb219161815f1d7336e26a686ab654e4ce468c6cf20463274e162a7b4ffdR116

@hostep
Copy link
Contributor

hostep commented Dec 11, 2024

Thanks Jisse!

@wilfriedwolf
Copy link
Contributor Author

Thanks from me too, sorry for the inconveniences caused.

@jissereitsma
Copy link
Contributor

No worries, I'm happy that we could work together to improve things!

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