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

Fix possible null error in search #8149

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

core23
Copy link
Member

@core23 core23 commented Jan 9, 2024

Subject

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

Changelog

### Fixed
- Fix possible null error in search

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear where there is an issue with the fact query can be null in the template https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/views/Core/search.html.twig

@core23
Copy link
Member Author

core23 commented Jan 9, 2024

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
Copy link
Member

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 #1855
Shouldn't we just update the template to if query is not null ?

@VincentLanglet
Copy link
Member

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

@core23
Copy link
Member Author

core23 commented Jan 10, 2024

Done

@VincentLanglet
Copy link
Member

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
Copy link
Member Author

core23 commented Jan 20, 2024

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 VincentLanglet merged commit f14a2b1 into sonata-project:4.x Jan 20, 2024
24 checks passed
@VincentLanglet
Copy link
Member

Thanks

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

@core23 core23 deleted the search-null-error branch January 30, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants