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

Add support for Symfony 3.3 #201

Merged
merged 3 commits into from
Apr 12, 2017
Merged

Add support for Symfony 3.3 #201

merged 3 commits into from
Apr 12, 2017

Conversation

pierredup
Copy link
Contributor

Since symfony/symfony#22010 there is now an extra argument to the translator constructor, which means the $options parameter moves one index up

@monteiro
Copy link
Collaborator

@pierredup thank you very much for your PR. Thanks to you we are supporting the next version of Symfony.

@monteiro monteiro merged commit f5bc07b into willdurand:master Apr 12, 2017
@pierredup pierredup deleted the patch-2 branch April 12, 2017 09:33
@craigh
Copy link
Contributor

craigh commented Oct 23, 2020

Hello @pierredup and @monteiro

I am trying to use this Bundle with Symfony 5.2-Beta2 and this line referencing the old number 3 arg is not working.

@pierredup I am wondering why you were merging the 3 arg with the 4 arg? can we just eliminate that line?

@stof
Copy link
Contributor

stof commented Oct 23, 2020

The code should not be merging both arguments, but reading the right one.

@stof
Copy link
Contributor

stof commented Oct 23, 2020

see Incenteev/translation-checker-bundle#11 for a solution

@pierredup
Copy link
Contributor Author

It only merges the values to check for the resource_files option in either the 3rd or 4th argument in the compiler pass. It doesn't pass those merged options back to to the service

@stof
Copy link
Contributor

stof commented Oct 23, 2020

@pierredup yeah, but this break if the other argument is not the options argument

@craigh
Copy link
Contributor

craigh commented Oct 23, 2020

see #293

@pierredup
Copy link
Contributor Author

pierredup commented Oct 23, 2020

@stof how does it break? Was there another change in the signature in versions after 3.3?

@pierredup
Copy link
Contributor Author

Okay I just read through all the linked issues. The issue here is not with SF >= 3.3, but rather 5.2 where the arguments are not resolved yet by the time this compiler pass runs.
And since this bundle don't support SF < 3.4 anymore, I think the compiler pass can be updated to just rely on the new signature and does'n have to worry about the old signature

@craigh
Copy link
Contributor

craigh commented Oct 23, 2020

this argument:

The issue here is not with SF >= 3.3, but rather 5.2 where the arguments are not resolved yet by the time this compiler pass runs

is opposite this argument:

I think the compiler pass can be updated to just rely on the new signature and does'n have to worry about the old signature

if the issue is the timing, then the order is correct. If the issue is the new signature order then timing doesn't matter.

@pierredup
Copy link
Contributor Author

pierredup commented Oct 24, 2020

In Symfony < 5.2, the 4th argument (3rd index) was an array. In Symfony 5.2, that changed to be an instance of Symfony\Component\DependencyInjection\Argument\AbstractArgument.

This compiler pass runs before the Symfony\Component\Translation\DependencyInjection\TranslatorPass, which replaces the AbstractArgument with the array of loader ids.

this argument:

The issue here is not with SF >= 3.3, but rather 5.2 where the arguments are not resolved yet by the time this compiler pass runs

is opposite this argument:

I think the compiler pass can be updated to just rely on the new signature and does'n have to worry about the old signature

if the issue is the timing, then the order is correct. If the issue is the new signature order then timing doesn't matter.

What I meant with the new signature, is the one from Symfony 3.3, not 5.2. So if this bundle wanted to keep support for Symfony < 3.3, then either the compiler pass needs to be updated to run after the Symfony compiler pass, so that the arguments can be resolved before this pass runs, or this check can be changed to not merge the arrays. But this bundle dropped support for Symfony <= 3.3 in #271, so the pass needs to be updated to just check for the 4th index and doesn't have to cater for the 3rd index anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants