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

PageExtension without deprecated InitRuntimeInterface #1436

Merged

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Jul 8, 2022

Subject

I am targeting this branch, because i don't know if that would count as BC.

Closes #{put_issue_number_here}.

it fixes:

The "Sonata\PageBundle\Twig\Extension\PageExtension" class implements "Twig\Extension\InitRuntimeInterface" that is deprecated since Twig 2.7, to be removed in 3.0.

The Thing about $env->getGlobals()['app']->getRequest()->getPathInfo();, my gut is telling me that RequestStack would be cleaner, but that wouldn't be BC anymore if this one might be.

Changelog

### Changed
- PageExtension doesn't need InitRuntimeInterface anymore

@Hanmac Hanmac mentioned this pull request Jul 8, 2022
17 tasks
@eerison
Copy link
Contributor

eerison commented Jul 8, 2022

@Hanmac can you open to 3.x? we should remove the deprecation from 3.x and merge into 4.x

@Hanmac
Copy link
Contributor Author

Hanmac commented Jul 8, 2022

@Hanmac can you open to 3.x? we should remove the deprecation from 3.x and merge into 4.x

@eerison you mean this changes should be BC for 3.x? or do you mean twig 3.x?

@eerison
Copy link
Contributor

eerison commented Jul 8, 2022

@Hanmac can you open to 3.x? we should remove the deprecation from 3.x and merge into 4.x

@eerison you mean this changes should be BC for 3.x? or do you mean twig 3.x?

well because this deprecations are in 3.x, and to upgrade from 3.x to 4.x, the guy need to solve all deprecations and after that he is safe to migrate to next major.

for this reason we should solve the deprecations into the 3.x branch and after that merge into 4.x

@Hanmac Hanmac force-pushed the twigPageExtensionWithoutInterface branch from a78f5ef to 4c1d74b Compare July 8, 2022 09:42
@SonataCI
Copy link
Collaborator

SonataCI commented Jul 8, 2022

Could you please rebase your PR and fix merge conflicts?

@Hanmac Hanmac changed the base branch from 4.x to 3.x July 8, 2022 09:54
@VincentLanglet
Copy link
Member

I disagree with @eerison, this should be done in 4.x.

In 3.x, we only to add deprecation about what will be changed/removed in 4.x about the PageBundle.
But there is no need to solve all the symfony/twig deprecation directly in 3.x. Especially because some of them require BC-break.

Here, not extending/implementing a class/interface is a BC-break. Sure there is almost no chance it will break someone code ; but we should stay with our BC-policy.

The strategy for the user will be the following

  • Bump to SonataPage 4.x which will be compatible with Symfony 4 and 5 and Twig 2 and 3.
  • Bump to Symfony 5.
  • Bump to Twig 3.

@eerison
Copy link
Contributor

eerison commented Jul 8, 2022

hmmm I see, But in this case who uses sonata page bundle 3.x never will solve all deprecations, because some of them are inside of sonata page code.

Then what do you say, is we do not need to care about deprecations in 3.x that was introduced from symfony, just solve them in the next major.

@Hanmac Hanmac force-pushed the twigPageExtensionWithoutInterface branch from 4c1d74b to 41a98a8 Compare July 8, 2022 11:04
@Hanmac Hanmac changed the base branch from 3.x to 4.x July 8, 2022 11:04
@eerison
Copy link
Contributor

eerison commented Jul 8, 2022

For me it's fine :)

There is a comment that you need to solve 👀

@VincentLanglet
Copy link
Member

But in this case who uses sonata page bundle 3.x never will solve all deprecations, because some of them

It's not an issue.
People should just solve direct deprecations, not indirect ones.

VincentLanglet
VincentLanglet previously approved these changes Jul 8, 2022
@VincentLanglet VincentLanglet requested review from jordisala1991 and a team July 8, 2022 13:55
@VincentLanglet VincentLanglet merged commit f669175 into sonata-project:4.x Jul 9, 2022
@VincentLanglet
Copy link
Member

Thanks

@Hanmac Hanmac deleted the twigPageExtensionWithoutInterface branch July 10, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants