Skip to content

Fix warning: Illegal string offset 'hex' by adding ternary fallback to white#2110

Closed
bassplayer7 wants to merge 5 commits intodompdf:developfrom
bassplayer7:fix-warning-hex
Closed

Fix warning: Illegal string offset 'hex' by adding ternary fallback to white#2110
bassplayer7 wants to merge 5 commits intodompdf:developfrom
bassplayer7:fix-warning-hex

Conversation

@bassplayer7
Copy link
Copy Markdown

No description provided.

@bsweeney bsweeney changed the base branch from master to develop March 9, 2020 22:34
@bsweeney
Copy link
Copy Markdown
Member

bsweeney commented Mar 9, 2020

Can you rebase your branch on the develop branch?

@bsweeney
Copy link
Copy Markdown
Member

bsweeney commented Mar 9, 2020

Do you have some sample HTML + CSS that exhibits the issue?

@Mellthas
Copy link
Copy Markdown
Member

Mellthas commented Mar 5, 2021

No HTML/CSS, but I can reproduce the warning with manually setting styles:

$style = new Style(new Stylesheet(new Dompdf()));
$style->border_top_color = "rgb(1,2,3,4,5)";
var_dump($style->get_border_top_color()); /* null */
var_dump($style->border_top_color); /* rgb(1,2,3,4,5) */
var_dump($style->get_border_top()); /* 1px none r, Warning: Illegal string offset 'hex' in ... */
var_dump($style->border_top); /* 1px none #000000FF */

So there are some inconsistencies to how the getters and property accessors work. get_border_top() calls _get_border("top") which returns the invalid color value, leading to the string-offset access on a string. border_top running through __get() somehow falls back to black for the color though. If you switch the order of the last two lines, the warning is gone, because the fallback black seems to be saved.

@Mellthas
Copy link
Copy Markdown
Member

Mellthas commented Mar 5, 2021

So I think the commit as-is is probably not the right thing to do.

@bsweeney
Copy link
Copy Markdown
Member

bsweeney commented Mar 5, 2021

Good analysis. I've been focusing on issues reproducible during rendering, but we should avoid issues like this one even in direct-usage scenarios.

@Mellthas
Copy link
Copy Markdown
Member

Mellthas commented Mar 5, 2021

An example reproducing the issue is in #2367 (comment).

@bsweeney
Copy link
Copy Markdown
Member

bsweeney commented Sep 1, 2021

Per example in issue 2367 if a named color is known but not translated (e.g. transparent) the error is raised. Though I still can't trigger the error in the method modified in this PR. Also, the $color variable is referenced multiple times and if we're tweaking the logic here should we also tweak it in the other locations?

Anyway, I would expect this particular case would have a valid array since we're retrieving the color through the style color getter. So I think maybe we can fix this without the ternary by fixing the getter.

@bsweeney
Copy link
Copy Markdown
Member

bsweeney commented Sep 2, 2021

I think the small change I made in #2536 works better for the expected return values to address this issue.

@bsweeney bsweeney closed this Sep 11, 2021
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.

TypeError: Cannot access offset of type string on string (src/Css/Style.php) in 1.0.1 and 1.0.2

6 participants