hofff / contao-social-tags

GNU Lesser General Public License v3.0
2 stars 5 forks source link

string cant be null error #19

Closed MrFastDie closed 3 years ago

MrFastDie commented 3 years ago

fix for

[2021-05-18 13:57:54] app.CRITICAL: An exception occurred. {"exception":"[object] (Symfony\Component\Debug\Exception\FatalThrowableError(code: 0): Type error: Argument 2 passed to Hofff\Contao\SocialTags\EventListener\Hook\NewsReaderListener::onGetContentElement() must be of the type string, null given, called in /var/www/html/staging/vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php on line 484 at /var/www/html/staging/vendor/hofff/contao-social-tags/src/EventListener/Hook/NewsReaderListener.php:43)"} []

dmolineus commented 3 years ago

A content element should always generate a string, so I don't think the fix is needed. Furthermore the broken content element has to be fixed.

MrFastDie commented 3 years ago

A content element should always generate a string, so I don't think the fix is needed. Furthermore the broken content element has to be fixed.

I don't fully agree here

Because first of all I don't get any information about which element needs to get fixed

And that problem occured on a client system which I don't maintain. But thinking of that creating a content element without a string is posible - so I would like to see that fix in anyways.

I think we both could aggree on "it should always have" declarations are not a habit.

dmolineus commented 3 years ago

Following the type documentation of a frontend module and content element, it must generate a string. (See https://github.com/contao/contao/blob/e44fcbcc25bacde033e9919a079b777105381cdf/core-bundle/src/Resources/contao/modules/Module.php#L204-L208). I know it's a weak type information, as it's part of the legacy code of Contao. But still implementations has to stick to it, otherwise it will lead to unforseen errors like this one.

MrFastDie commented 3 years ago

The major difference is that Contao is using phpdoc to create typehints.

This file on the other hand is using declare(strict_types=1); - which makes the hints to be forced to explicit types. As Contao is not using this feature it's still possible to get this error...

So we should either remove the strict_types according to the contao code, or adjust the string to actually be null.

fritzmg commented 3 years ago

I agree with @dmolineus, the Content Element in question needs to be fixed to return a string.

dmolineus commented 3 years ago

Thanks for your PR. I won't merge it as it would avoid symptoms but not fixing the root issue,