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

Support SF7 #509

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Support SF7 #509

merged 3 commits into from
Aug 22, 2024

Conversation

Growiel
Copy link
Contributor

@Growiel Growiel commented Aug 20, 2024

Hello,

Continuing from: #502

The root of the issue is that in the test context, we're missing the Symfony\Bundle\WebProfilerBundle\Profiler\CodeExtension that provides the needed file_link Twig Filter.

This is the result of dd(array_keys($this->twig->getExtensions())); right before the problematic template (@Translation/WebUI/show.html.twig) is rendered:

array:14 [
  0 => "Twig\Extension\CoreExtension"
  1 => "Twig\Extension\EscaperExtension"
  2 => "Twig\Extension\YieldNotReadyExtension"
  3 => "Twig\Extension\OptimizerExtension"
  4 => "Symfony\Bridge\Twig\Extension\ProfilerExtension"
  5 => "Symfony\Bridge\Twig\Extension\TranslationExtension"
  6 => "Symfony\Bridge\Twig\Extension\AssetExtension"
  7 => "Symfony\Bridge\Twig\Extension\RoutingExtension"
  8 => "Symfony\Bridge\Twig\Extension\YamlExtension"
  9 => "Symfony\Bridge\Twig\Extension\HttpKernelExtension"
  10 => "Symfony\Bridge\Twig\Extension\HttpFoundationExtension"
  11 => "Twig\Extension\DebugExtension"
  12 => "Translation\Bundle\Twig\TranslationExtension"
  13 => "Translation\Bundle\Twig\EditInPlaceExtension"
]

As we can see, the extension mentioned in the begining is missing.

I do not know why it behaves like this all of a sudden, I'm guessing something changed in later versions of the bundle.

As for the fix, I simply added the bundle to the ones installed in the "test app" that's used to run the Functional Tests.

@Growiel
Copy link
Contributor Author

Growiel commented Aug 20, 2024

@bocharsky-bw I believe you might be the one with the rights to launch the automated tests, locally they run, but I don't test all the versions obviously.

@bocharsky-bw
Copy link
Member

Hey @Growiel!

Nice catch! Now tests are completely green.

However, from the test job output I see we're still installing a few packages from 6.4 instead of 7

  • symfony/yaml
  • symfony/dependency-injection
  • symfony/config

I think it still may prevent people from upgrading to Sf7 completely. Do you have any ideas about how to allow Sf 7 for those packages too? I might be a conflict with other dependencies that we should bump

@Growiel
Copy link
Contributor Author

Growiel commented Aug 21, 2024

Hello again,

I think we're good to go with the packages as is.

According to packagist, the three bundles support Symfony 7 starting with version 6.4.0 of the packages.
However, version 7.0.0 and up of the packages drop Symfony 5.4 support.

If we want to keep the bundle compatible with SF 5.4, we can't upgrade these packages.

I installed a fresh SF7.1 and then composer required my fork, installation worked perfectly and I could access the WebUI and ProfilerUI pages just fine.

@bocharsky-bw
Copy link
Member

I think that's more complex than you think... the problem is that we don't install those 7.1 versions of the packages I mentioned, and so we do not test this project on those packages which may lead to some potential issues. In theory, yes, it should work... at least this bundle should be installed on Symfony 7 now, but it may have some bugs, i.e. it would be good to confirm with tests that the bundle works on the latest Sf7 packages.

I dug a bit here, and it seems those packages were not installed because we're on older versions of some test packages:

  • nyholm/symfony-bundle-test
  • matthiasnoback/symfony-config-test
  • matthiasnoback/symfony-dependency-injection-test

We need to upgrade those test packages first (maybe something else) in order to allow Composer to install all Sf7 packages and fully test this bundle against Symfony 7 versions. So, the problem is not in allowing legacy Sf 5.4 but in not allowing newer versions of some bundles.

@bocharsky-bw
Copy link
Member

If you have time, it would be good to finish it. If not, I think we could merge this at least to allow Sf7 at least, but that would be not ideal as our tests do not test the project against all Sf7 versions, some packages are still on 6.4

@Growiel
Copy link
Contributor Author

Growiel commented Aug 22, 2024

Hello,

I upgraded matthiasnoback/symfony-dependency-injection-test and matthiasnoback/symfony-config-test which were the ones restricting us to 6.4 versions of the packages.

For matthiasnoback/symfony-dependency-injection-test I only upgraded to v5 and not v6 because v6 drops support for PHPUnit 9 and I didn't feel like upgrading to PHPUnit 10.

I had to add a requirement for PHPUnit 9.6 since composer was now installing 10 by default.

Let me know.

@bocharsky-bw
Copy link
Member

Hey @Growiel ,

Thank you for the additional changes! Sounds good to stay on PHPUnit 9, and I'm OK with matthiasnoback/symfony-dependency-injection-test upgrade up to v5 - it seems that's enough to unblock the remaining Sf 7 packages.

Cheers!

@bocharsky-bw bocharsky-bw merged commit 0b3eaaa into php-translation:master Aug 22, 2024
8 checks passed
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.

2 participants