Feature/stop warning on broken lists

[ross-ritchey]

Hello,

I was working on a Magento 2 site that has MPDF integrated to make PDF's out of product listings.

A good number of the product descriptions on the site have LI's without a parent UL/OL (bad HTML - I know - but will take time to fix and can't guarantee a future user won't do this again)

When the PDF is going through MPDF, the BlockTag class has an issue where a WARNING level error is thrown in this instance. The warning is that an array key doesn't exist -> list_style_type (and presumably list_style_image and list_style_position as well - just errors before those)

This pull adds a check to see if any of those are not set and calls _setListMarker with default values ('disc' for list_style_type, the defaults noted higher in the file for the others).

This prevents the WARNING from being thrown - allowing a system like Magento 2 (which stops script execution when a WARNING is thrown) to properly finish generating the PDF.

Adding a pull request as I thought this fix might be helpful for others.

[finwe]

Hi. Thanks for the PR. But it obscures an underlying problem which could be somewhat seen without it. With it, the user will most likely never know something is wrong.

I'm hesitant to merge it without addressing this and adding an appropriate test suite. More generally, I'm always hesitant to merge just a bunch of isset checks around things that cause a warning.

I'd say a better course of action than to slap this band-aid on the warning issuing code without fixing it properly (either by non-wellformed HTML warnings or by initializing the list structures) would be to surround mpdf execution with a custom error handler catching and suppressing the warning.

[ross-ritchey]

I agree with the sentiment here -> though my counterpoint is that there actually isn’t something wrong in these instances. Browsers and MPDF properly render list items without the surrounding UL/OL all the time. Yeah – that HTML it might not pass a validation check – but as far as the users are concerned it is working properly. Prior to PHP 8.1 this type of issue sent out a NOTICE, not a WARNING. IE – it was not an issue. PHP moved to warning’s for these to push developers to write clean code. This code is throwing a warning because it improperly assumes that the array keys will exist at that point -> IE – it is not written properly. I am open to the fact that my solution might not be the most ideal – however with this issue still in the code base, MPDF is not actually compatible with PHP 8.1.Thanks

[finwe]

Notices and Warnings are the same in this context - neither of those stop the excecution of the code unless bumped up to an Exception by a 3rd party library, in your case Magento (I don't know about Magento, but most frameworks raise the Exception in development mode only, in production mode, the warning would be just logged and not shown and the PDF would be generated properly). So mPDF is really compatible with PHP 8.1, it's only the error_reporting level that is the factor here (and it eventually should not stop the execution of the code in production environments).

Still, this change:

• obscures information for the user that was present before
  • call it a soft BC break, even, there were reported notices/warnings that helped users to fix their HTML
  • I'm musing if a logger call with the information about possibly non-wellformed HTML would suffice for me, I don't know yet
• is missing a test suite
• and most importantly IMHO could/should be solved by better data structure holding list information - with all properties initialized so that the isset calls are not necessary.
  • A need for this would be also obscured by this change and would be much harder to get back to

[ross-ritchey]

Fair points. In terms of the obscuring information -> I would argue a logger call about possibly non-wellformed HTML would actually be better than the current solution of allowing the array keys warning to be thrown. As a user I had to dig into the library to figure out what the problem was – determined that it was probably improperly formed HTML – and then looked at the source HTML to confirm that. A logger warning actually pointing me at the source HTML as the problem would have been much simpler solution without the risk of error_reporting levels causing the PDF to just not render. I can take a look at the testing suite used here. Wasn’t sure if a test was really necessary. I agree that ideally properly formed HTML would be used -> but that cannot be guaranteed in any scenario with users. MPDF is used in many places where the content being rendered is written by people without technical backgrounds – increasing the odds that this kind of issue will be in the HTML.