xyNNN / GoogleTagManagerBundle

Google Tag Manager Bundle for Symfony 2
https://github.com/xyNNN/GoogleTagManagerBundle
GNU Lesser General Public License v3.0
27 stars 15 forks source link

Fix code-level breaking changes for Symfony 5 #35

Closed conciergerie-numerique closed 4 years ago

conciergerie-numerique commented 4 years ago

Hello!

I've tested the new version of the bundle on a real Symfony 5 project and there were some issues.

  1. Commit 121783c. In Symfony code base some deprecated event classes were removed in Symfony 5 @see https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching. Their API contract used by the bundle didn't change, some just removing type hint is enough to fix it without breaking support for older Symfony versions.

  2. Commit 6c8a1d8. The bundle is incompatible with Twig >=3.0 because of usage of Twig_Extension, Twig_SimpleFunction and Twig_Environment classes. @see https://twig.symfony.com/doc/1.x/deprecated.html. The fix is simple but it will make the bundle incompatible for Twig 1 users. I've explicitly set Twig compatibility equal to the one of Symfony 2.8 and incompatible with Twig >=3.0.

  3. Commit 7e7ba8d. The bundle uses php traits that were introduced in php 5.4, so I adjusted the dependency accordingly.

  4. There were some unused imports.

conciergerie-numerique commented 4 years ago

Commit 5d003f1. PHPUnit was trying to construct Twig_Environment object while mocking which failed.

conciergerie-numerique commented 4 years ago

Commit 7f4e444. Removing php 5.3 from Travis CI target configuration because the php minimal required version is 5.4 after 7e7ba8d.

xyNNN commented 4 years ago

Thanks for your contribution and investigation.

So the 2.6.0 release is not working with Symfony 5.0, right? Your commits will fix this issue, so that we can create a new release (2.7.0) for the next release which is compatible with Symfony 5.0 and don't have any breaking changes included. Therefore we'a broken release (my fault) with 2.6.0.

I will try to use this bundle with Symfony 4.x and 5.x to ensure that it's proper working.

conciergerie-numerique commented 4 years ago

Indeed, the version 2.6.0 isn't working with Symfony 5.

It's my fault. I should have tested properly before submitting a pull request, sorry for that, it is unprofessional from my part.

New changes do fix incompatibility issues due to the removal of some deprecated code.

If you could take a look at changes before creating a new release, it would be wonderful.

Thank you for taking your time to support this bundle!

Best wishes, Valentin Sviridov

Le mar. 21 janv. 2020 à 09:18, Philipp Bräutigam notifications@github.com a écrit :

Thanks for your contribution and investigation.

So the 2.6.0 release is not working with Symfony 5.0, right? Your commits will fix this issue, so that we can create a new release (2.7.0) for the next release which is compatible with Symfony 5.0 and don't have any breaking changes included. Therefore we'a broken release (my fault) with 2.6.0.

I will try to use this bundle with Symfony 4.x and 5.x to ensure that it's proper working.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/xyNNN/GoogleTagManagerBundle/pull/35?email_source=notifications&email_token=ANVX7MDMYRAM5RIUYCCCMITQ62VWFA5CNFSM4KJGUWJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJO3YMY#issuecomment-576568371, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANVX7MCRQGY3OU33LK6HEA3Q62VWFANCNFSM4KJGUWJA .

xyNNN commented 4 years ago

I've tested it with Symfony 4.x and 5.x and looks pretty good! What do you think about, that I merge this into master, you check it again in your project with dev-master as Composer constraint dependency and if all works fine, I create a new release.

What do you think @valentinsviridov ?

valentinsviridov commented 4 years ago

It is OK for me.

Le mar. 21 janv. 2020 à 10:58, Philipp Bräutigam notifications@github.com a écrit :

I've tested it with Symfony 4.x and 5.x and looks pretty good! What do you think about, that I merge this into master, you check it again in your project with dev-master as Composer constraint dependency and if all works fine, I create a new release.

What do you think @valentinsviridov https://github.com/valentinsviridov ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/xyNNN/GoogleTagManagerBundle/pull/35?email_source=notifications&email_token=ABLF6D7F53MMDKATXBFGV7LQ63BLDA5CNFSM4KJGUWJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJPEYWY#issuecomment-576605275, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLF6D7BT4TMX5OTMOCELBLQ63BLDANCNFSM4KJGUWJA .

xyNNN commented 4 years ago

Okay, let's go and give me feedback :) Thanks mate!

valentinsviridov commented 4 years ago

I've tested the version dev-master#e293357 with two projects running Symfony 4.4.2 and Symfony 5.0.2 and the bundle seems to work fine. I think that you can release a new version.

On Tue, Jan 21, 2020 at 11:05 AM Philipp Bräutigam notifications@github.com wrote:

Okay, let's go and give me feedback :) Thanks mate!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/xyNNN/GoogleTagManagerBundle/pull/35?email_source=notifications&email_token=ABLF6D2CBVQJCCQ4PF3ZN5LQ63CH3A5CNFSM4KJGUWJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJPFRQA#issuecomment-576608448, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLF6D4AE56SCE4ZVLQ2EC3Q63CH3ANCNFSM4KJGUWJA .

xyNNN commented 4 years ago

See https://github.com/xyNNN/GoogleTagManagerBundle/releases/tag/2.7.0