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

[SoftDeletable] Fix ArrayAccess deprecation on FieldMapping #2856

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AlboCode
Copy link

[Unreleased]

Fixed

@AlboCode
Copy link
Author

I was worried about the compatibility to doctrine/orm ^2.14.0
If the check is unnecessary can be removed

@fgelmo
Copy link

fgelmo commented Aug 22, 2024

Very nice

@mbabker
Copy link
Contributor

mbabker commented Aug 22, 2024

You don't need Composer's API here to do a version check when you're already making a "is the meta an instance of this object" check as well. The instance check is more important than the version check because both the ORM and MongoDB ODM are supported.

And while this will fix this one instance of the deprecation, there are literally dozens of cases that need to be fixed if the intent is to silence all deprecations.

@AlboCode
Copy link
Author

You don't need Composer's API here to do a version check when you're already making a "is the meta an instance of this object" check as well. The instance check is more important than the version check because both the ORM and MongoDB ODM are supported.

And while this will fix this one instance of the deprecation, there are literally dozens of cases that need to be fixed if the intent is to silence all deprecations.

Ok, it seems fine without checking the version.
I'm looking only by the SoftDeletable part and there are not so many cases, and I was noticing code where there is already the check (maybe because much recent) like Gedmo\SoftDeleteable\Mapping\Event\Adapter\ORM::getRawDateValue($mapping) line 65.

private function getRawDateValue($mapping)
    {
        $datetime = $this->clock instanceof ClockInterface ? $this->clock->now() : new \DateTimeImmutable();
        $type = $mapping instanceof FieldMapping ? $mapping->type : ($mapping['type'] ?? '');

I think to keep consistency in the codebase, it is better to align every part where FieldMapping is used, instead of a mixed situation.

Gedmo\SoftDeleteable\Mapping\Event\Adapter\ORM::getDateValue
@mbabker
Copy link
Contributor

mbabker commented Aug 23, 2024

I'm looking only by the SoftDeletable part and there are not so many cases, and I was noticing code where there is already the check (maybe because much recent) like

Because those private methods originally had the $mapping argument typehinted as an array, which had to be changed to support ORM 3 so the code was just updated to account for that change, not because "oh no a deprecation!".

I think to keep consistency in the codebase, it is better to align every part where FieldMapping is used, instead of a mixed situation.

I can't argue that, but as I've mentioned elsewhere, there are dozens of locations where the mapping data is used and needs to account for the MongoDB ODM structure (arrays), the ORM 2.x structure (arrays), and the ORM 3.x structure (DTOs with a deprecated ArrayAccess implementation for B/C). Just fixing these one at a time because someone noticed messages in their deprecation logs is going to end up in more inconsistency until they're all fixed.

I'm not in a spot to stop someone from making these patches as they need to land eventually, but IMO fixing these deprecations (which is just adding extra noise to logs, not breaking anything) is a rather low priority compared to some of the other stuff on the issues list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants