php-translation / symfony-bundle

Symfony integration for Translations
MIT License
327 stars 94 forks source link

Support SF7 #509

Closed Growiel closed 2 months ago

Growiel commented 2 months ago

Hello,

Continuing from: https://github.com/php-translation/symfony-bundle/pull/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 commented 2 months ago

@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 commented 2 months ago

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

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 commented 2 months ago

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 commented 2 months ago

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:

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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!