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

Uncaught TypeError with PHP 8.3 and id attribute #93

Open
lee-peuker opened this issue Dec 6, 2023 · 4 comments
Open

Uncaught TypeError with PHP 8.3 and id attribute #93

lee-peuker opened this issue Dec 6, 2023 · 4 comments

Comments

@lee-peuker
Copy link

lee-peuker commented Dec 6, 2023

What is this feature about (expected vs actual behaviour)?

Running into an error after when minifying <div id="test"></div> after updating to PHP 8.3. I expected to get the minified html.

How can I reproduce it?

Script to reproduce issue (using PHP 8.3.0):

<?php declare(strict_types=1);

use voku\helper\HtmlMin;

include_once(__DIR__ . '/../vendor/autoload.php');

$vokuMinifier = new HtmlMin;

$vokuMinifier->minify('<div id="test"></div>');

Error:

Fatal error: Uncaught TypeError: Cannot assign null to property DOMElement::$id of type string in /vendor/voku/simple_html_dom/src/voku/helper/AbstractSimpleHtmlDom.php:174
Stack trace:
#0 /vendor/voku/html-min/src/voku/helper/HtmlMinDomObserverOptimizeAttributes.php(146): voku\helper\AbstractSimpleHtmlDom->__set('id', NULL)
#1 /vendor/voku/html-min/src/voku/helper/HtmlMin.php(1688): voku\helper\HtmlMinDomObserverOptimizeAttributes->domElementAfterMinification(Object(voku\helper\SimpleHtmlDom), Object(voku\helper\HtmlMin))
#2 /vendor/voku/html-min/src/voku/helper/HtmlMin.php(1667): voku\helper\HtmlMin->notifyObserversAboutDomElementAfterMinification(Object(voku\helper\SimpleHtmlDom))
#3 /vendor/voku/html-min/src/voku/helper/HtmlMin.php(1370): voku\helper\HtmlMin->minifyHtmlDom('<div id="test">...', false)
#4 /scripts/htmlMin.php(9): voku\helper\HtmlMin->minify('<div id="test">...')
#5 {main}
  thrown in /vendor/voku/simple_html_dom/src/voku/helper/AbstractSimpleHtmlDom.php on line 174

Does it take minutes, hours or days to fix?

No idea

Any additional information?

in AbstractSimpleHtmlDom.php:174 there already seems to be some catch in place, if this is extended with $nameOrig === 'id' the error is fixed, but not sure if this is wanted.

                    // INFO: Cannot assign null to property DOMNode::* of type string
                    if ($nameOrig === 'prefix' || $nameOrig === 'textContent') {
                        $value = (string)$value;
                    }
@ArnaudLigny
Copy link

Hello, I'm also trying to update my app to PHP 8.3 and got the same problem:

TypeError: Cannot assign null to property DOMElement::$id of type string

/home/runner/work/Cecil/Cecil/vendor/voku/simple_html_dom/src/voku/helper/AbstractSimpleHtmlDom.php:174
/home/runner/work/Cecil/Cecil/vendor/voku/html-min/src/voku/helper/HtmlMinDomObserverOptimizeAttributes.php:146
/home/runner/work/Cecil/Cecil/vendor/voku/html-min/src/voku/helper/HtmlMin.php:1688
/home/runner/work/Cecil/Cecil/vendor/voku/html-min/src/voku/helper/HtmlMin.php:1667
/home/runner/work/Cecil/Cecil/vendor/voku/html-min/src/voku/helper/HtmlMin.php:1370

https://github.com/Cecilapp/Cecil/actions/runs/7120600738/job/19388168813?pr=1676#step:14:659

@repat
Copy link

repat commented Dec 12, 2023

@lee-peuker @ArnaudLigny The error is in the underlying package voku/simple_html_dom, not in this package.

@voku There is already a fix PR for it: voku/simple_html_dom#106
Could you please merge it so we can use your package in PHP 8.3? :) Thanks!

@ConnectGrid
Copy link

While I agree that the SimpleHtmlDom needs the fix, shouldn't this one "watch" for such violations, too? Should it not detect if id was trying to be assigned a value of null, rather than just blindly trying to set every attribute to null?

The null assignment is happening on Line 146 in the voku\helper\HtmlMinDomObserverOptimizeAttributes\domElementAfterMinification() method of this library, which is why it's easy to think that this library needs the fix.

@thierrydrc
Copy link

Hope that the merge will be done soon 🙏

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

No branches or pull requests

5 participants