search

sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Fix possible null error in search #8149

Closed core23 closed 8 months ago

core23 commented 9 months ago

Subject

I am targeting this branch, because this is a patch.

Changelog

### Fixed
- Fix possible null error in search
core23 commented 9 months ago

It is an issue, I got several exception in my projects.

In the template we only check if the query variable is defined and not false. A null value is a defined value so the blocks are rendered.

VincentLanglet commented 9 months ago

It is an issue, I got several exception in my projects.

What is the exception you get ?

The block result doesn't seems to use the query variable https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/views/Block/block_search_result.html.twig

In the template we only check if the query variable is defined and not false. A null value is a defined value so the blocks are rendered.

'' value will also be defined so block will be rendered in the same way.

Also, it's kinda weird to have this defined and not false check, since we use query in the translation too. https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/views/Core/search.html.twig#L14

it was made in https://github.com/sonata-project/SonataAdminBundle/pull/1855 Shouldn't we just update the template to if query is not null ?

VincentLanglet commented 9 months ago

Btw if you rebase, I fix the phpstan/rector build

core23 commented 8 months ago

Done

VincentLanglet commented 8 months ago

It is an issue, I got several exception in my projects.

What is the exception you get ?

The block result doesn't seems to use the query variable 4.x/src/Resources/views/Block/block_search_result.html.twig

In the template we only check if the query variable is defined and not false. A null value is a defined value so the blocks are rendered.

'' value will also be defined so block will be rendered in the same way.

Also, it's kinda weird to have this defined and not false check, since we use query in the translation too. 4.x/src/Resources/views/Core/search.html.twig#L14

it was made in #1855 Shouldn't we just update the template to if query is not null ?

I appreciate you re-request my review but I have a lot of question unanswered...

core23 commented 8 months ago

It is an issue, I got several exception in my projects.

What is the exception you get ?

I receive a TypeError, as the null value is passed from the template to the SearchHandler:

/XXX/vendor/sonata-project/admin-bundle/src/Search/SearchHandler.php:38
/XXX/vendor/sonata-project/admin-bundle/src/Block/AdminSearchBlockService.php:68
/XXX/vendor/sonata-project/block-bundle/src/Block/BlockRenderer.php:46
/XXX/vendor/sonata-project/block-bundle/src/Templating/Helper/BlockHelper.php:162
/XXX/var/cache/prod/twig/98/98cd481aa2f0c78834272cc52e85c07d.php:105

The block result doesn't seems to use the query variable https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/views/Block/block_search_result.html.twig

In the template we only check if the query variable is defined and not false. A null value is a defined value so the blocks are rendered.

'' value will also be defined so block will be rendered in the same way.

Also, it's kinda weird to have this defined and not false check, since we use query in the translation too. https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/views/Core/search.html.twig#L14

it was made in #1855 Shouldn't we just update the template to if query is not null ?

To be safe we could fix it in the template as well, but this won't help if you are using a custom template. The root cause is the block, not the template.

VincentLanglet commented 8 months ago

Thanks

Do you have the rights to make a release ? Or I will do it next week