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

Typed property must not be accessed before initialization #67

Closed
armpogart opened this issue Feb 4, 2020 · 18 comments
Closed

Typed property must not be accessed before initialization #67

armpogart opened this issue Feb 4, 2020 · 18 comments
Labels
magic Structural changes, new features, performance. type:question Further information is requested

Comments

@armpogart
Copy link

Try to add php 7.4 property typing to any of the Entity examples from docs and persist it with db, you will get that exception.

For the reference see also doctrine issue and corresponding PR. As far as I see the issue is partially resolved, see this one.

Not sure whether the issue in Doctrine was only for id (i.e. primary keys), but here we have the issue for any typed field in Entity.

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 5, 2020

@alexndr-novikov can you comment? I know we are widely using 7.4, not sure is it the same issue we solved earlier or something new.

@armpogart thank you for the report.

@armpogart
Copy link
Author

@wolfy-j Actually the issue is not directly in this repo, but the dependency you use. You run following code which itself uses Zend Hydrator and there the following line raises the issue.

It's due to the fact that php 7.4 property value can't be accessed even with reflection if it is not initialized, but the fix is rather easy as ReflectionProperty class has new methods for that and I see better fix than Doctrine one.

The problem is I don't see how to apply the fix to this repo as it is in direct dependency. Do I need to report there?
And the second problem is the fix will raise minimum php requirements to 7.4, or can we just do some method_exists check to see if we are on php 7.4 and only do the checks there?

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 5, 2020

Yes, you might need to report in Zend/Hydrator. Also, check ocramius/generated-hydrator, it is viable alternative (and it works faster). I think PHP7.4 issues were fixed by @Ocramius

This is the example https://github.com/adrianmiu/forked-php-orm-benchmark/blob/master/cycleorm/CycleOrmTestSuiteWithGeneratedMapper.php

Since Zend is kinda dead we might need to switch to Laminas on a later date. In general, hydration/extraction code has nothing to do with the rest of ORM so it's very easy to change the implementation.

@armpogart
Copy link
Author

Issue on ocramius/generated-hydrator for reference. Will open an issue on laminas/laminas-hydrator (since zend is already depracated and repo is archived) and update here.

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 6, 2020

I really hope it's not going to end with our own hydration/extraction implementation (hi @vvval). So far it seems that there needs a way to quickly detect if the property has been initialized. I wonder if isset is working.

@armpogart
Copy link
Author

@wolfy-j Are you following discussion on both repos where I referenced this issue?
Do you agree with the statement that it is implementation problem? (there are comments on that on both cases, though I don't agree with them).

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 7, 2020

Not really, if I want to get values from the entity - I have to get values from the entity. If they won't fix it on the library level we will have to write our own hydration/extractor with extra check if the property has been initialized and skip its extraction (orm will be fine with that). But I can't express how much I do not want to write another hydration library.

@wolfy-j wolfy-j added type:question Further information is requested magic Structural changes, new features, performance. labels Feb 8, 2020
@rauanmayemir
Copy link
Contributor

Not only zend-hydrator fails to work with typed properties, its ReflectionHydrator somehow ignores the default constructor (so there's no way to safely bypass the exception without making all of the properties nullable).

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 16, 2020

It's normal practice to bypass the constructor invocation in Hydration (same as Doctrine).

I'm planning to seriously take a look at this issue during following few weeks.

@Ocramius
Copy link

Released https://github.com/Ocramius/GeneratedHydrator/releases/tag/3.1.0, if you wanna give it a spin.

It still uses an old zendframework/zend-hydrator version for now. I would suggest sending upstream patches to laminas-hydrator meanwhile.

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 23, 2020

Well, according to Marco comment Ocramius/GeneratedHydrator#124 this can’t be fixed as there design bug.

I can only propose not to declare auto incremental ids in entities at all or stick to uuid or make them nullable.

@Ocramius
Copy link

Ocramius commented Feb 23, 2020

Yes: of your identifier doesn't exist until after persistence, it is to be null, and the entity mutable.

Otherwise you just left a landmine there, for somebody else to uncover.

EDIT: s/nullable/mutable

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 23, 2020

Going to add it to PHP 7.4 specific documentation article. Thank you, Marco.

@armpogart
Copy link
Author

armpogart commented Feb 23, 2020

@wolfy-j If I understood correctly, you agree that if not nullable typed property is not initialized that's a design issue. While I agree on the case of identifier (PK) can be nullable until after persistence it is set. What about FK keys which can be also null after persistence (when nulls are allowed), how they would be differentiated?

So also my comment on laminas issue tracker.

@Ocramius
Copy link

If the association is nullable, why isn't it mapped as such in the entity?

@armpogart
Copy link
Author

@Ocramius Ok, for making an example simpler.
Suppose we have some content field in db that can be null. When using typed properties it would be logical to have it null by default (same goes for any FK that can be null), but how we will differentiate whether it is null by default or it is set to null on purpose (after ORM read operation)?

Now, I'm little bit lost. I need to think more about this...

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 25, 2020

Why there should be a difference? In theory, your code should know nothing about ORM (since we are not talking about AR) so your code should operate relying only on itself.

@Ocramius
Copy link

It doesn't even need to be null by default: if an association is nullable, it can be hydrated as null when the association isn't set.

@wolfy-j wolfy-j closed this as completed Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
magic Structural changes, new features, performance. type:question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants