Fix for duplicating parent relation columns to child tables in JTI

peldax commented 2 weeks ago

Partial fix of https://github.com/cycle/annotated/issues/97

🔍 What was changed

I removed initialization of relations if the declaring class differs from currently processed reflection class. This solves a case from issue above, where BelongsTo relation column were duplicated to child tables as well.

📝 Checklist

Partially solves #97, but only the sub-issue
How was this tested:
- [x] Tested manually
- [ ] Unit tests added

📃 Documentation

Not needed, this is expected behaviour.

NOTE

This solution is just a hotfix identifying the problem. The real solution will be more complex as it probably needs to deduce whether the declaring class is really the parent entity or some other class in the hierarchy between the parent entity and the child entity..

peldax commented 2 weeks ago

In ad29886 I added a logic to identify whether the property belongs to current entity or any parent entity in the hierarchy. This renders the note above as resolved.

peldax commented 2 weeks ago

Tests in CI fail due to phpunit deprecation, it is still needed to add a new testcases to cover the relevant usecases though.

roxblnfk commented 2 weeks ago

To fix tests try to set mysql:8.0.37 there https://github.com/cycle/annotated/blob/7dad356336ee70ef1f6e7b750274d4d215a691f1/tests/docker-compose.yml#L14

peldax commented 2 weeks ago

Sure, right now I am working on a fix to #101 as it is somewhat related.

peldax commented 2 weeks ago

In 90989a4 I implemented a logic to lookup an entity which truly owns the property.

This solves various issues:

Relations from JTI parent are not included in the child entity in case of JTI (the original purpose of this PR)
- Relations from base class which is not an entity should be included normally.
Columns from parent/base classes are correctly included in the table schema.
- The mechanism for property ownership is prepared for JTI with theoretically any number of nested levels.
- This closes #101 as well.

peldax commented 2 weeks ago

@roxblnfk Hi, I have downgraded mysql to 8.0 and also resolved the PHPUnit deprecation. However, there are 2 failing tests related to the proxyFieldWithAnnotation and I am currently not sure what that means. Would you please help me to resolve the final bits in order to mainstream the fixes introduced in this PR?

roxblnfk commented 2 weeks ago

Hi. It looks like this warning (Note) has gone off 😃

You can adjust this test for the new behavior if we talk about this:

Failed asserting that 'value' is null.
tests/Annotated/Functional/Driver/Common/Inheritance/JoinedTableTestCase.php:128

roxblnfk commented 2 weeks ago

Hm.. The inheritance tests aren't passed even on the main branche

Looks like this assertion has unnecessary check isset($parent) https://github.com/cycle/annotated/blob/7dad356336ee70ef1f6e7b750274d4d215a691f1/src/TableInheritance.php#L69

But I`m not sure

peldax commented 2 weeks ago

Hi. It looks like this warning (Note) has gone off 😃

Yes, this is a side effect of this PR - to support base classes that are not entitites, but declare columns. Base classes can be placed anywehere in the table inheritance hierarchy and on as many levels as needed.

peldax commented 2 weeks ago

Hm.. The inheritance tests aren't passed even on the main branche

Looks like this assertion has extra check isset($parent)

https://github.com/cycle/annotated/blob/7dad356336ee70ef1f6e7b750274d4d215a691f1/src/TableInheritance.php#L69

But I`m not sure

I am also not sure about that part. The edits from maintainers are allowed, so feel free to jump into my branch.

peldax commented 2 weeks ago

Hi @roxblnfk , thanks for looking into it! The metadata reader provider changes were made due to phpunits deprecation, which was introduced in 10.5.18.

peldax commented 1 week ago

Hi, I would love to see this patch to land into a release. :shipit:

roxblnfk commented 1 week ago

Hello. Do these changes require additional testing? If not, then I'll review it again and merge.

peldax commented 1 week ago

Well, there is always a possibility to add additional automated tests. I am running this patch locally and so far it has been working well.

cycle / annotated