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

[BUGFIX] Skip any style elements with null value, but still remove them #1226

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Sep 10, 2023

Psalm indicates that DOMNode::nodeValue could be null, perhaps for entirely empty <style> elements.

If such are encountered during pre-processing, still remove them from the DOM, but don't append their contents to the CSS to be inlined with extra line-breaks.

@JakeQZ JakeQZ added bug php Pull requests that update Php code type annotations Either in PHP or the DocBlock for Psalm, or for issues found by Psalm or other tools re type-safety labels Sep 10, 2023
@JakeQZ JakeQZ added this to the 8.0.0 milestone Sep 10, 2023
@JakeQZ JakeQZ self-assigned this Sep 10, 2023
@JakeQZ JakeQZ changed the title [BUGFIX] Skip any style elements with null value, but stil remove them [BUGFIX] Skip any style elements with null value, but still remove them Sep 10, 2023
src/CssInliner.php Outdated Show resolved Hide resolved
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

LGTM.

@oliverklee
Copy link
Contributor

@JakeQZ Could you please rebase, resolve the merge conflicts, and repush? Thanks!

JakeQZ and others added 2 commits September 12, 2023 00:29
Psalm indicates that `DOMNode::nodeValue` could be `null`,
perhaps for entirely empty `<style>` elements.

If such are encountered during pre-processing, still remove them from the DOM,
but don't append their contents to the CSS to be inlined with extra line-breaks.
…y providing extra information.

Good suggestion.  I agree.  It's `null|string` (nothing else), so much clearer to test for the non-null type.

Co-authored-by: Oliver Klee <github@oliverklee.de>
@oliverklee oliverklee merged commit 08970a6 into main Sep 12, 2023
48 checks passed
@oliverklee oliverklee deleted the bugfix/null-nodevalue branch September 12, 2023 06:48
nlemoine pushed a commit to nlemoine/emogrifier that referenced this pull request Sep 19, 2023
…em (MyIntervals#1226)

Psalm indicates that `DOMNode::nodeValue` could be `null`,
perhaps for entirely empty `<style>` elements.

If such are encountered during pre-processing, still remove them from the DOM,
but don't append their contents to the CSS to be inlined with extra line-breaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug php Pull requests that update Php code type annotations Either in PHP or the DocBlock for Psalm, or for issues found by Psalm or other tools re type-safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants