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

history_revision_timestamp.html.twig not rendering on dev #7217

Closed lortuno closed 3 years ago

lortuno commented 3 years ago

Hi, I have a problem with the admin-bundle from version 3.97 on (version 3.96 worked) In this release the layouts for history_revision_timestamp.html.twig were changed, and the field_description variable started being mandatory (although in the next included twig, display_datetime.html.twig, this is optional and checked with an if clause) {%- include '@SonataAdmin/CRUD/display_datetime.html.twig' with { value: revision.timestamp, format: format, timezone: timezone, } only -%} So using the audit bundle (https://packagist.org/packages/sonata-project/entity-audit-bundle) when I go to see revisions the layout breaks. This is because the fielddescription variable is not defined there. I see that on all display* layouts this error is silenced but this history layout does not start with the prefix. In production this is working because the layout renders ignoring errors but in dev it's causing us trouble and we had to override the template. (https://github.com/sonata-project/SonataAdminBundle/blob/3.x/CHANGELOG.md)

image

lortuno commented 3 years ago

Can you include an update to have this fix, please?

VincentLanglet commented 3 years ago

Why would you use https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Resources/views/CRUD/history_revision_timestamp.html.twig without a field description ?

Use '@SonataAdmin/CRUD/display_datetime.html.twig' directly instead.

lortuno commented 3 years ago

Hi, @VincentLanglet . The thing is this behaviour comes from vendor, I didn't modify anything. history_revision includes display_datetime but this is called internally. When the audit bundle uses this layout for some reason the field_description is not defined. Still, the problem is the check of field_description is on the display layout and not on the previous layout, so I think it would be better if the bundle itself modified the layout so it can work without throwing errors.

VincentLanglet commented 3 years ago

The call is made https://github.com/sonata-project/SonataAdminBundle/blob/01677b25381aa3c3d87dfaf7fe8699d9bc4744f3/src/Resources/views/CRUD/base_history.html.twig#L38

This is the only usage of this template, so seems like it's supposed to be used without a field_description. The code of https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Resources/views/CRUD/history_revision_timestamp.html.twig should be changed to

{%- include '@SonataAdmin/CRUD/display_datetime.html.twig' with { value: revision.timestamp } -%}

Can you make a PR ?

lortuno commented 3 years ago

@VincentLanglet I'm no expert on the bundle to make this change. I just wanted to let developers know that this change from 3.96 to 3.97 and following versions break the layout due to the above. I don't know why this was changed, maybe there is a reason for it...

lortuno commented 3 years ago

So can you review this in future releases?

VincentLanglet commented 3 years ago

So can you review this in future releases?

I can't give you a deadline. Best way to get this fixed is to open a PR, I basically explain the way to solve the issue here: https://github.com/sonata-project/SonataAdminBundle/issues/7217#issuecomment-847037720