Fix for duplicating parent relation columns to child tables in JTI

[peldax]

Partial fix of #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 [Bug]: HasOne overrides nullability of BelongsTo #97, but only the sub-issue
• How was this tested:
  • 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]

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]

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

[roxblnfk]

To fix tests try to set mysql:8.0.37 there

annotated/tests/docker-compose.yml

Line 14 in 7dad356

image: mysql:latest

[peldax]

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

[peldax]

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 [Bug]: Columns declared in non-entity classes are ignored 🐛  #101 as well.

[peldax]

@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]

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]

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

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

annotated/src/TableInheritance.php

Line 69 in 7dad356

\assert($child->getRole() !== null && $entity !== null && isset($parent));

But I`m not sure

[peldax]

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]

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

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

annotated/src/TableInheritance.php

Line 69 in 7dad356

\assert($child->getRole() !== null && $entity !== null && isset($parent));

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]

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]

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

[roxblnfk]

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

[peldax]

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.

[roxblnfk]

I've added tests for this case and there is an issue when an entity extended from another entity without STI or JTI.

In thos case all the relations of the parent entity have to be cloned into the child class.

[roxblnfk]

There is TableInheritance generator. It might be better to clean unnecessary relations there in cases STI or JTI

[peldax]

Thanks for the review.

You are right, it is quite a nasty use case though. I will take a look at it in the evening,
It makes sense to clear the entity in the inheritance generator, I will try to move the logic there.

The column ownership use cases are alright?

[roxblnfk]

Yeah, columns look great. Thank you.

[peldax]

@roxblnfk Hi,
Feel free to take over from here and alter the patch according to your needs, I am no longer using Cycle and will not invest time to implement the comments from your latest review.

[roxblnfk]

@roxblnfk Hi, Feel free to take over from here and alter the patch according to your needs, I am no longer using Cycle and will not invest time to implement the comments from your latest review.

Thank you for bringing this to my attention. And thank you for the work you've done.