FriendsOfSymfony / FOSOAuthServerBundle

A server side OAuth2 Bundle for Symfony
1.09k stars 451 forks source link

Replacing the Twig deprecated spaceless tag with the apply tag introduced in Twig 2.9 #619

Closed crmpicco closed 3 years ago

crmpicco commented 5 years ago

Template change looks good. However, on lower twig versions this would fail, since we don't enforce any version constraints there. You should add a conflict for twig/twig there to forbid any versions < 1.40 and 2.9:

"conflict": {
    "twig/twig": "<1.40|>=2.0,<2.9"
}

Last but not least, please target the 1.6 branch for this change as we want to fix this for 1.x customers as well.

I'll hold off on merging this until we get the build fixed, but thanks for your contribution!

Thanks for the quick reply and thanks for the comments.

I'm happy to update composer.json to add the conflict in there, however can you advise how I target the 1.6 branch? I note that it is listed as a stale branch.

alcaeus commented 5 years ago

can you advise how I target the 1.6 branch? I note that it is listed as a stale branch.

When you edit the PR title, you can also change the base branch of the PR. However, since the master branch contains a bunch of changes that shouldn't be in 1.6, this would include a bunch of commits we don't want in 1.6. This requires you to rebase your branch, either by running git rebase --onto (of which I've never understood the syntax for), or by resetting the branch and cherry-picking the single commit (this is what I normally do for branches with few commits): git reset --hard 1.6 && git cherry-pick ee10a1141dc563f0df00601d47974b4691726fc6

You might get conflicts on cherry-pick, which you can then resolve and finish using git commit. After this you force-push and change the base branch.

If you don't see yourself doing the above, please let me know and I can do it for you, but I'm sure you can do it 👍

crmpicco commented 5 years ago

Hi @alcaeus , thanks for the info. I'm not really familiar with your workflow there, however I have force-pushed as advised. Thanks

crmpicco commented 5 years ago

PR updated for composer conflict change.

crmpicco commented 5 years ago

Hi @alcaeus, does this look ok to you?

deguif commented 3 years ago

Closing as this was fixed with #652