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

[AllBundles] Fix BC break after upgrading to sensio/framework-extra-bundle 5.0 #2045

Merged

Conversation

acrobat
Copy link
Member

@acrobat acrobat commented Jun 30, 2018

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets closes #2039 (can be closed as the downgrade is not required anymore with this fix)

Upgrading to sensio/framework-extra-bundle 5.0 caused a bc break for camelcase template paths which were not explicitly configured (Symfony would guess the template path for the empty @Template() annotations. This is fixed by explicitly settings the template path for these cases.

I've also remove some unused @Template annotations as these actions returned a redirect so the annotation was unused.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

@acrobat acrobat force-pushed the bc-upgrade-framework-extra-bundle branch from ab887b1 to 3f05b43 Compare June 30, 2018 11:58
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

@@ -438,7 +436,6 @@ private function createAndRedirect(Request $request, $folderId, $type, $redirect
*
* @Route("create/modal/{folderId}/{type}", requirements={"folderId" = "\d+", "type" = ".+"}, name="KunstmaanMediaBundle_media_modal_create")
* @Method({"GET", "POST"})
* @Template()
*
Copy link
Contributor

Choose a reason for hiding this comment

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

method 'createAndRedirect' can still return an array. In that case it could break. So either 'createAndRedirect' should be adapted or a default template should be provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'm not sure this method is actually used anymore. I can only find 1 usage in the code, but I don't know yet how to "trigger" that usage. Currently there is no corresponding twig template for this action, so I will look into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This action was not used as a "GET" anywhere only as a POST for the add file/image/video popup. So I've removed GET as allowed method. Also this code couldn't have worked before as the is no template createModal.html.twig or create_modal.html.twig

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing a three year old bug

@acrobat acrobat force-pushed the bc-upgrade-framework-extra-bundle branch from 3f05b43 to a688e66 Compare July 11, 2018 19:53
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

@acrobat acrobat force-pushed the bc-upgrade-framework-extra-bundle branch from a688e66 to 7fa19df Compare July 23, 2018 20:50
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @acrobat, your PR passed all our requirements.

Thank you for contributing!

@acrobat
Copy link
Member Author

acrobat commented Jul 23, 2018

This PR is ready for review/merge

@sandergo90 sandergo90 merged commit 3965203 into Kunstmaan:master Jul 26, 2018
Symfony 4 support automation moved this from In progress to Done Jul 26, 2018
@acrobat acrobat deleted the bc-upgrade-framework-extra-bundle branch July 26, 2018 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants