IBX-8470: Upgraded codebase to Symfony 6 #447

alongosz · 2024-10-30T14:10:41Z

Caution

This is a part of bigger set of changes, to be merged together when ready

❗ Remove TMP commits before merge ❗

🎫 Issue	IBX-8470

Related PRs:

Description:

ℹ️ Browser tests failure is expected until all OSS packages are upgraded.

Composer packages bump:

symfony/* to ^6.4
jms/translation-bundle to ^2.4
friendsofsymfony/jsrouting-bundle to ^3.5

Essential changes:

Declared strict return types for class methods extending/overriding 3rd party packages, that are either already there in bumped version or will be there for sure in the future
Aligned strict types for method arguments overriding 3rd party packages
Fixed Symfony DI PHP API setDeprecated usage (it will most likely disappear after merging deprecation removal changes)
Fixed octal value representation in Yaml (e.g. 0644 -> 0o644)
Removed obsolete configuration parameters from Symfony DI semantic config
Aligned usage of 3rd party PHP API with the upgraded versions of those packages
Aligned tests with both current changes and 3rd party packages changes
Replaced usage of deprecated utf8_decode PHP function with mb_convert_encoding

DX:

Added strict types to some properties in the vicinity of or related to the upgraded code
- BinaryFile is an example which gave most trouble due to us relying on uninitialized properties, fixed now
Added or fixed PHPDocs
Refactored pieces of touched code to be more readable (helping also understanding what's going on there while trying to fix it after the version bump)
Improved code quality of tests

Maintenance:

[PHPStan] Added to the baseline serializer issues (postponed to SF 7, where PropertyNormalizer is final)
[PHPUnit] Configured exact numbers of self, direct, and indirect deprecation to have more control over what's changed.

QA:

TBD if we want to merge it if all green or test everything manually before.

Documentation:

Dropped obsolete Ibexa\Core\MVC\Symfony\Security\InteractiveLoginToken. Seems this was some Symfony 2 auth leftover. Mentioned in the User authentication Doc needs to be dropped or changed as well (there should be a task for this already, just wasn't able to find it ATM).
Dropped from \Ibexa\Core\MVC\Symfony\Security\UserWrapped obsolete methods: getPassword, getSalt, getUsername and implemented the new one getUserIdentifier.

.github/workflows/ci.yaml

Steveb-p · 2024-11-13T09:10:52Z

src/bundle/Core/DependencyInjection/Compiler/InjectEntityManagerMappingsPass.php

+                $class = $metadataDriverDefinition->getClass();
+                if (null !== $class && (str_contains($class, 'yml') || str_contains($class, 'xml'))) {


This raises my eyebrow.

We are checking the class declared for metadata driver. Which means we are, in effect, checking that Doctrine\ORM\Mapping\Driver\YamlDriver contains yml string?

Can you check at runtime what is actually happening here? Since I'm not sure this actually works as intended.

I don't expect us to "fix it" here. But if you're touching this code (I assume since you've seen PHPStan report it) I'd rather have it checked.

This raises my eyebrow.

We are checking the class declared for metadata driver. Which means we are, in effect, checking that Doctrine\ORM\Mapping\Driver\YamlDriver contains yml string?

Can you check at runtime what is actually happening here? Since I'm not sure this actually works as intended.

I don't expect us to "fix it" here. But if you're touching this code (I assume since you've seen PHPStan report it) I'd rather have it checked.

I can only guess as the test that is there (\Ibexa\Tests\Bundle\Core\DependencyInjection\Compiler\InjectEntityManagerMappingPassTest) probably checks assumed values. This was an extension point and I wasn't sure how it was used.

Keep in mind that ORM used to allow keeping mappings in either Yaml or XML format. Maybe it's related to that?

I can drop it, but only if you're sure that no longer applies. At runtime OOTB we have only annotation type present.

Yes, it can be either XML, YAML or annotations/attributes. It's just that class name would not contain yml string, since the driver is named YamlDriver.

Worst case leave it as is, but it should be checked. It's a potential bug that was probably unnoticed, because majority of projects are using annotations.

src/bundle/Core/Fragment/DecoratedFragmentRenderer.php

src/bundle/Core/Routing/DefaultRouter.php

src/bundle/IO/BinaryStreamResponse.php

src/lib/MVC/Symfony/Routing/Generator/UrlAliasGenerator.php

…p-parser v5

Postponed for Symfony 7 upgrade

…const

…ding

Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>

sonarcloud · 2024-11-13T15:11:13Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.4% Duplication on New Code

See analysis details on SonarQube Cloud

konradoboza

As mentioned in one of my comments below, I would switch more towards constructor property promotion (not necessarily within the scope of this PR).

konradoboza · 2024-11-15T08:47:32Z

src/bundle/IO/BinaryStreamResponse.php


    /**
-     * Constructor.
+     * @param array<string, array<string>> $headers An array of response headers
+     * @param bool $public Files are public by default


Suggested change

* @param bool $public Files are public by default

* @param bool $public files are public by default

konradoboza · 2024-11-15T08:53:45Z

src/lib/MVC/Symfony/Translation/ValidationErrorFileVisitor.php


-    /** @var \Doctrine\Common\Annotations\DocParser */
-    private $docParser;
+    private DocParser $docParser;


Why (this is one of the examples of course) this cannot be constructor-promoted property? I feel like we should incorporate this rule more consistently.

Given the amount of changes in scope, I assume we are not doing this - effectively a code style change - in this PR. The primary goal is to allow Symfony 6 to work, and everything else is optional.

If we have time to do so - great.

I am ok with that, just wanted to highlight as I tried to move into that directions when I was tinkering with the code related to security layer.

src/lib/Resources/settings/settings.yml

github-actions bot assigned mikadamczyk, alongosz and tischsoic Oct 30, 2024

alongosz force-pushed the ibx-8470-symfony-6 branch 2 times, most recently from 568db5c to a1ac641 Compare October 31, 2024 10:49

alongosz added Doc needed The changes require some documentation Feature New feature request Work in progress Rebase required labels Oct 31, 2024

alongosz force-pushed the ibx-8470-symfony-6 branch 4 times, most recently from 36125d0 to 93ad6ef Compare November 6, 2024 14:48

alongosz marked this pull request as ready for review November 7, 2024 09:30

alongosz force-pushed the ibx-8470-symfony-6 branch from 93ad6ef to 443188a Compare November 8, 2024 12:14

alongosz mentioned this pull request Nov 12, 2024

IBX-8470: Upgraded codebase to Symfony 6 ibexa/solr#78

Open

alongosz force-pushed the ibx-8470-symfony-6 branch 2 times, most recently from 6440557 to fba6ef0 Compare November 12, 2024 15:02

alongosz mentioned this pull request Nov 12, 2024

IBX-8470: Upgraded codebase to Symfony 6 ibexa/test-core#13

Open

alongosz removed the Work in progress label Nov 12, 2024

alongosz requested a review from a team November 12, 2024 16:01

Steveb-p reviewed Nov 13, 2024

View reviewed changes

alongosz added 8 commits November 13, 2024 12:13

[Composer] Bumped symfony-related packages requirements to ^6.4

c470095

[Composer] Bumped 3rd party requirements for Symfony 6

0110012

[PHPDoc] Improved StorageConnectionFactory annotations for Symfony 6

2dde708

[Tests] Upgraded ExceptionMessageTemplateFileVisitorTest for nikic/ph…

af8d749

…p-parser v5

[Tests] Refactored BaseRenderStrategyTest for Symfony 6

9c8980e

Refactored fragment renderers for Symfony 6

02d9410

[Tests] Refactored fragment renderer tests for Symfony 6

310980a

[Tests] Aligned SerializerStub methods with Symfony 6

1008bb1

alongosz and others added 18 commits November 13, 2024 12:13

[PHPStan] Added to baseline classes extending final PropertyNormalizer

5dabd31

Postponed for Symfony 7 upgrade

[Tests] Aligned IOService tests with strict types changes and Symfony 6

736c140

Fixed BinaryFile Value members' strict types to reflect usage

6c03513

Changed IOService::loadBinaryFile to throw an exception for abs. paths

d73e7a3

[Tests] Aligned AliasGeneratorTest with BinaryFile strict requirements

cd60ffa

Aligned Symfony DI Extension implementations with Symfony 6 deprecations

0e6ee56

[Tests] Improved quality of IbexaRepositoryInstallerBundleTest class

5a30419

[PHPUnit] Updated new deprecation counts coming from Symfony 6

65031ea

Aligned Symfony Bundle implementations with Symfony 6 deprecations

c94cbce

[Tests] Dropped obsolete semantic config options from tests setup

126d3c2

[Tests] Aligned BinaryBaseStorageTest with BinaryFile strict types

e7f60ec

Fixed octal values in semantic config for Symfony 6

b24788b

[Tests] Improved SiteAccessAwareVisibilityConverterTest strict octal …

68d86b6

…const

[Tests] Replaced deprecated function utf8_decode with mb_convert_enco…

6479b28

…ding

[PHPStan] Removed resolved issues from the baseline

76705fe

[TMP] Required dev branch of ibexa/doctrine-schema

f391591

[TMP][CI] Required SF 6 dev branch of ibexa/solr

298aeab

Improved code quality

e293296

Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>

alongosz force-pushed the ibx-8470-symfony-6 branch from fba6ef0 to e293296 Compare November 13, 2024 15:10

This was referenced Nov 14, 2024

IBX-8470: Upgraded codebase to Symfony 6 ibexa/notifications#12

Open

IBX-8470: Upgraded codebase to Symfony 6 ibexa/admin-ui-assets#24

Open

IBX-8470: Upgraded codebase to Symfony 6 ibexa/core-persistence#21

Open

konradoboza reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-8470: Upgraded codebase to Symfony 6 #447

IBX-8470: Upgraded codebase to Symfony 6 #447

alongosz commented Oct 30, 2024 •

edited

Loading

Steveb-p Nov 13, 2024

alongosz Nov 13, 2024

Steveb-p Nov 13, 2024

sonarcloud bot commented Nov 13, 2024

konradoboza left a comment •

edited

Loading

konradoboza Nov 15, 2024

konradoboza Nov 15, 2024

Steveb-p Nov 15, 2024

konradoboza Nov 15, 2024

		$class = $metadataDriverDefinition->getClass();
		if (null !== $class && (str_contains($class, 'yml') \|\| str_contains($class, 'xml'))) {

	* @param bool $public Files are public by default
	* @param bool $public files are public by default

IBX-8470: Upgraded codebase to Symfony 6 #447

Are you sure you want to change the base?

IBX-8470: Upgraded codebase to Symfony 6 #447

Conversation

alongosz commented Oct 30, 2024 • edited Loading

Related PRs:

Description:

Composer packages bump:

Essential changes:

DX:

Maintenance:

QA:

Documentation:

Steveb-p Nov 13, 2024

Choose a reason for hiding this comment

alongosz Nov 13, 2024

Choose a reason for hiding this comment

Steveb-p Nov 13, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Nov 13, 2024

Quality Gate passed

konradoboza left a comment • edited Loading

Choose a reason for hiding this comment

konradoboza Nov 15, 2024

Choose a reason for hiding this comment

konradoboza Nov 15, 2024

Choose a reason for hiding this comment

Steveb-p Nov 15, 2024

Choose a reason for hiding this comment

konradoboza Nov 15, 2024

Choose a reason for hiding this comment

alongosz commented Oct 30, 2024 •

edited

Loading

konradoboza left a comment •

edited

Loading