8000 Remove reference to LiipThemeBundle and dispatch event instead by rogamoore · Pull Request #7004 · sulu/sulu · GitHub
[go: up one dir, main page]
Skip to content

Remove reference to LiipThemeBundle and dispatch event instead #7004

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 15, 2023
Merged

Remove reference to LiipThemeBundle and dispatch event instead #7004

merged 2 commits into from
Feb 15, 2023

Conversation

rogamoore
Copy link
Contributor
@rogamoore rogamoore commented Feb 13, 2023 •
edited
Loading
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #
Related issues/PRs #
License MIT

What's in this PR?

The SuluThemeBundle switched to SyliusThemeBundle some time ago. See sulu/SuluThemeBundle#19

This PR updates the ValidateWebspacesCommand to use the correct components to set the theme and use it when checking for the existence of templates.

This PR updates the ValidateWebspacesComman 8000 d to dispatch a PreRenderEvent instead which causes the SuluThemeBundle to correctly set the correct theme.

Why?

Without this fix, when using themes, the command will report missing templates even if the templates exist.

@alexander-schranz
Copy link
Member

@rogamoore the problem is we should not have any requirement to SyliusThemeBundle inside Sulu Core. So it would be more abstract and extendable if we would inject eventDispatcher and dispatch am event which will be thrown and SuluThemeBundle could listen to it. This way even custom theming and custom template loaders can be used and they can just listen to that event to make the validator pass.

@alexander-schranz
Copy link
Member

We also could use existing event from the Preview the PreRenderEvent: https://github.com/sulu/SuluThemeBundle/blob/d08918569fe5cb261a16efa5f816fbe9f0153ee7/EventListener/SetThemeEventListener.php#L67 to be thrown here as that event already used by the SuluThemeBundle to set the correct theme.

@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Sulu label Feb 14, 2023
@rogamoore
Copy link
Contributor Author

@alexander-schranz I was thinking the same. Actually the whole bug is based on the hard coded reference to LiipThemeBundle. This PR was meant as a quick fix because I stumbled on this problem when starting to use themes.

If you think we could re-use the PreRenderEvent, I could change the PR to dispatch the event as it has been done here:
https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/PreviewBundle/Preview/Renderer/PreviewRenderer.php#L187
PS: I'm wondering now what the TODO comment is about :)

Regarding tests I guess we could check somehow if the event has been dispatched here in testExecute: https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/PageBundle/Tests/Functional/Command/ValidateWebspacesCommandTest.php#L54

@alexander-schranz
Copy link
Member

We did keep the LiipthemeBundle in it because it was that case in the past. But with the Min Requirement to Symfony 5.4 it can completely removed as it can not longer exist in Symfony 5.4. And for SyliusTheme we should find a more general solution so we have not a dependency to a service which is actually not required by the composer.json of sulu.

PS: I'm wondering now what the TODO comment is about :)

I'm even not sure about the comment. I even think that it is with the change to SyliusThemeBundle again required to dispatch that event. But I would need to test it if we remove that event if theme is still correctly be used. But as it is still there I'm also fine to keep it if we have a usecase for it and remove the comment and deprecation.

Regarding tests I guess we could check somehow if the event has been dispatched here in testExecute:

From test point of view I think it make sense to check if the event was dispatched with correct webspace attribute.

@rogamoore rogamoore force-pushed the bugfix/webspace-validator branch from f6dd5f4 to feddf60 Compare February 14, 2023 17:16
@rogamoore
fix: remove reference to LiipThemeBundle and dispatch event instead
3826c2e 
fix: remove reference to LiipThemeBundle and dispatch event instead

fix: remove support for LiipThemeBundle and use SyliusThemeBundle instead
@rogamoore rogamoore force-pushed the bugfix/webspace-validator branch from feddf60 to 3826c2e Compare February 14, 2023 17:20
@rogamoore rogamoore changed the title fix: remove support for LiipThemeBundle and use SyliusThemeBundle instead remove reference to LiipThemeBundle and dispatch event instead Feb 14, 2023
@alexander-schranz alexander-schranz changed the title remove reference to LiipThemeBundle and dispatch event instead Remove reference to LiipThemeBundle and dispatch event instead Feb 15, 2023
@alexander-schranz
Copy link
Member
alexander-schranz commented Feb 15, 2023 •
edited
Loading

The CI errors seems unrelated to the changes here. I will have a look at it.

@alexander-schranz
Copy link
Member

I tested it out locally all works like expected for current stable and current 6.2@dev release. So the issue is somewhere in Symfony 6.3@dev branch, I created an issue in Symfony for it symfony/symfony#49387.

I also updated the UPGRADE.md file, it is a small bc break in the constructor but not worth investing here as I don't think somebody changed here anything.

@alexander-schranz alexander-schranz merged commit faee567 into sulu:2.5 Feb 15, 2023
@rogamoore
Copy link
Contributor Author

Thanks a lot @alexander-schranz !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature New functionality not yet included in Sulu
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rogamoore @alexander-schranz
0