Fix for duplicating parent relation columns to child tables in JTI

[peldax]

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]

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 https://github.com/cycle/annotated/blob/7dad356336ee70ef1f6e7b750274d4d215a691f1/tests/docker-compose.yml#L14

[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 #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) https://github.com/cycle/annotated/blob/7dad356336ee70ef1f6e7b750274d4d215a691f1/src/TableInheritance.php#L69

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)

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]

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. :shipit:

[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.

[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.

[roxblnfk]

For those interested in completing the fix: the code is saved in the fo-bar branch. Feel free to open a pull request to this branch.