IBX-8470: Upgraded codebase to Symfony 6 #447
Conversation
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
2 times, most recently
from
568db5c to
a1ac641
Compare
alongosz
added
Doc needed
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
4 times, most recently
from
36125d0 to
93ad6ef
Compare
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
from
93ad6ef to
443188a
Compare
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
2 times, most recently
from
6440557 to
fba6ef0
Compare
| $class = $metadataDriverDefinition->getClass(); | ||
| if (null !== $class && (str_contains($class, 'yml') || str_contains($class, 'xml'))) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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\YamlDrivercontainsymlstring?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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, it can be either XML, YAML or annotations/attributes. It's just that class name would not contain
ymlstring, since the driver is namedYamlDriver.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.
There was a problem hiding this comment.
XML mapping is very much alive and kicking, even in 4.0 DBAL (upcoming).
That being said, I'm okay with leaving it as is for now.
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
from
fba6ef0 to
e293296
Compare
|
|
||
| /** | ||
| * Constructor. | ||
| * @param array<string, array<string>> $headers An array of response headers | ||
| * @param bool $public Files are public by default |
There was a problem hiding this comment.
| * @param bool $public Files are public by default | |
| * @param bool $public files are public by default |
There was a problem hiding this comment.
Actually, why do we even bother with adding info that is already part of the declaration?
| * @param bool $public Files are public by default |
|
|
||
| /** @var \Doctrine\Common\Annotations\DocParser */ | ||
| private $docParser; | ||
| private DocParser $docParser; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Postponed for Symfony 7 upgrade
Co-Authored-By: Paweł Niedzielski <[email protected]>
alongosz
force-pushed
the
ibx-8470-symfony-6
branch
from
682d6d7 to
be8e99d
Compare
For the list of Pagerfanta changes see #462 --------- Co-Authored-By: Dawid Parafiński <[email protected]>
|
Caution
This is a part of bigger set of changes, to be merged together when ready
Related PRs:
Description:
ℹ️ Browser tests failure is expected until all OSS packages are upgraded.
Composer packages bump:
Essential changes:
setDeprecatedusage (it will most likely disappear after merging deprecation removal changes)0644->0o644)utf8_decodePHP function withmb_convert_encodingDX:
BinaryFileis an example which gave most trouble due to us relying on uninitialized properties, fixed nowMaintenance:
PropertyNormalizeris final)QA:
TBD if we want to merge it if all green or test everything manually before.
Documentation:
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).\Ibexa\Core\MVC\Symfony\Security\UserWrappedobsolete methods:getPassword,getSalt,getUsernameand implemented the new onegetUserIdentifier.