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.

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.

@Steveb-p I've checked it more closely now. At runtime there's unresolved parameter there %doctrine.orm.metadata.yml.class% or %doctrine.orm.metadata.xml.class% and that how and why it works. Very unstable, but I'd suggest getting rid of it with doctrine upgrade, since Yml and Xml mappings are deprecated.

XML mapping is very much alive and kicking, even in 4.0 DBAL (upcoming).

https://www.doctrine-project.org/projects/doctrine-orm/en/latest/reference/xml-mapping.html#xml-mapping

That being said, I'm okay with leaving it as is for now.

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

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

Actually, why do we even bother with adding info that is already part of the declaration?

Suggested change

* @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

Postponed for Symfony 7 upgrade

…const

…ding

Co-Authored-By: Paweł Niedzielski <[email protected]>

sonarcloud · 2024-12-11T15:54:46Z

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

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 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

This was referenced Nov 15, 2024

IBX-8470: Upgraded codebase to Symfony 6 ibexa/behat#125

Open

IBX-8470: Upgraded codebase to Symfony 6 ibexa/core-search#7

Open

IBX-8470: Upgraded codebase to Symfony 6 ibexa/cron#19

Open

alongosz and others added 27 commits December 11, 2024 14:58

Aligned UrlWildcardRouter with Symfony 6

3fc3d02

Aligned DefaultRouter with Symfony 6

eb31614

Aligned URLAlias router with Symfony 6 and improved code quality

efde037

Improved quality of BinaryStreamResponse and aligned with Symfony 6

b929461

[Security] Aligned UserWrapped with Symfony 6

49aadc8

[Tests] Aligned UserWrappedTest with Symfony 6

eb64479

[Cache] Aligned TransactionalInMemoryCacheAdapter with Symfony 6

236ae70

[PHPStan] Added to baseline classes extending final PropertyNormalizer

91dca96

Postponed for Symfony 7 upgrade

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

ef9761d

Fixed BinaryFile Value members' strict types to reflect usage

2a4adb9

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

d661788

[Tests] Aligned AliasGeneratorTest with BinaryFile strict requirements

02d9237

Aligned Symfony DI Extension implementations with Symfony 6 deprecations

009f2e7

[Tests] Improved quality of IbexaRepositoryInstallerBundleTest class

603b82f

[PHPUnit] Updated new deprecation counts coming from Symfony 6

8036654

Aligned Symfony Bundle implementations with Symfony 6 deprecations

8e3d339

[Tests] Dropped obsolete semantic config options from tests setup

d03351e

[Tests] Aligned BinaryBaseStorageTest with BinaryFile strict types

56a6dcb

Fixed octal values in semantic config for Symfony 6

5a3d0f4

[Tests] Improved SiteAccessAwareVisibilityConverterTest strict octal …

9e8f3bc

…const

[Tests] Replaced deprecated function utf8_decode with mb_convert_enco…

e6b26c3

…ding

Improved code quality

b0dfa14

Co-Authored-By: Paweł Niedzielski <[email protected]>

[PHPStan] Removed resolved issues from the baseline

1bb4b63

Aligned StructWrapperValidator with Symfony 6

3da8b80

Fixed property-read type of Values\Content\Content::@$fields

ba456aa

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

ff56247

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

be8e99d

alongosz force-pushed the ibx-8470-symfony-6 branch from 682d6d7 to be8e99d Compare December 11, 2024 15:46

[Tests] Adjusted Symfony deprecation helper config after the changes

6a25c07

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

alongosz Dec 11, 2024

Steveb-p Dec 12, 2024 •

edited

Loading

konradoboza left a comment •

edited

Loading

konradoboza Nov 15, 2024

Steveb-p Dec 12, 2024

konradoboza Nov 15, 2024

Steveb-p Nov 15, 2024

konradoboza Nov 15, 2024

sonarcloud bot commented Dec 11, 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:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steveb-p Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

konradoboza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 11, 2024

Quality Gate passed

alongosz commented Oct 30, 2024 •

edited

Loading

Steveb-p Dec 12, 2024 •

edited

Loading

konradoboza left a comment •

edited

Loading