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

[Timestampable] Prevent deprecated ArrayAccess on FieldMapping in doctrine/orm 3 #2834

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cngJo
Copy link

@cngJo cngJo commented Jul 1, 2024

During an upgrade to doctrine/orm 3 together with gedmo/doctrine-extensions 3.16.1, I noticed a lot of deprecation messages that seem to originate in this package, rather than my application code ;)

I verified this change by applying a non-backwards compatible version to in my vendor directory directly, and the deprecation messages went away.


I introduced a small helper method to avoid code duplication around the instanceof check. This can be removed when support for doctrine/orm 2 is dropped somewhere in the future and replaced with:

-$this->getType($mapping);
+$mapping->type;

@cngJo
Copy link
Author

cngJo commented Jul 1, 2024

Ah sorry ... I just found #2827 after the fact. Feel free to just close this PR if you're not interested in this change 🙂

@mbabker
Copy link
Contributor

mbabker commented Jul 1, 2024

Feel free to just close this PR if you're not interested in this change 🙂

The issue isn't necessarily whether we're interested in this type of change or not, but with as widespread as use of the mapping data in this package is, and how it needs to account for at least 3 different shapes (ORM 2.x arrays, ORM 3.x objects, and ODM 2.x arrays), my personal feeling is it just ends up adding a lot of code to this package which affects runtime performance with the only benefit of (trying to) be deprecation-free on a new install/update, especially as similar conditionals have to be introduced in a few dozen places. That's the reason I just used #2827 (comment) to not have to think about it in my own applications.

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.

3 participants