[Loggable] Allow composite identifiers

[MisatoTremor]

Composite identifiers with foreign entities

New version of #2128 as of main branch rename

[rotdrop]

Could this possibly be merged? IMHO it is an important feature to add support for composite identifiers.

[MisatoTremor]

Rebased and fixed PHP and XML code style

[MisatoTremor]

Well, finally adopted the changed test namespaces.

@phansys Could you give me a heads up, where this breaks BC, please? Maybe this can be changed

[phansys]

@phansys Could you give me a heads up, where this breaks BC, please? Maybe this can be changed

Sure: https://github.com/doctrine-extensions/DoctrineExtensions/pull/2171#discussion_r749102248.

[MisatoTremor]

Resolved conflicts and code style

@phansys Could you please verify this no longer breaks BC?

[phansys]

@phansys Could you please verify this no longer breaks BC?

I think the current changes respect BC, thank you :+1:

[MisatoTremor]

Resolved all requests

[MisatoTremor]

@phansys Hopefully I catched all missing type hints and imports. The PHPStan error seems to be unrelated to this PR

[phansys]

BTW, I think the changelog section should include an Added entry, as this is a new feature.

[MisatoTremor]

You are right of course, doesn't make sense to not mention it 😅

[codecov-commenter]

Codecov Report

Merging #2171 (3ac4c85) into main (0a97af5) will decrease coverage by 0.73%. The diff coverage is 89.74%.

@@             Coverage Diff              @@
##               main    #2171      +/-   ##
============================================
- Coverage     80.97%   80.24%   -0.74%     
- Complexity     3119     3121       +2     
============================================
  Files           153      153              
  Lines          8038     8007      -31     
============================================
- Hits           6509     6425      -84     
- Misses         1529     1582      +53     

Impacted Files	Coverage Δ
src/References/Mapping/Driver/Xml.php	0.00% <0.00%> (ø)
src/Loggable/Mapping/Driver/Annotation.php	77.77% <50.00%> (ø)
src/Loggable/Mapping/Driver/Yaml.php	62.71% <50.00%> (ø)
src/Loggable/LoggableListener.php	94.16% <75.00%> (ø)
...oggable/Document/Repository/LogEntryRepository.php	95.23% <100.00%> (+0.07%)	:arrow_up:
.../Loggable/Entity/Repository/LogEntryRepository.php	96.61% <100.00%> (ø)
src/Loggable/Mapping/Driver/Xml.php	83.33% <100.00%> (ø)
src/Loggable/Mapping/Event/Adapter/ORM.php	100.00% <100.00%> (ø)
src/Tool/Wrapper/EntityWrapper.php	100.00% <100.00%> (+2.32%)	:arrow_up:
src/Tool/Wrapper/MongoDocumentWrapper.php	88.63% <100.00%> (ø)
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0a97af5...3ac4c85. Read the comment docs.

[MisatoTremor]

Resolved conflicts.

Could you please give me a heads up why my changes might remove coverage here @phansys? Or should I ignore that?

[phansys]

Could you please give me a heads up why my changes might remove coverage here @phansys? Or should I ignore that?

I think it is due the way Codecov makes the comparison with the parent commit. I guess the coverage report will be more accurate after a rebasing and squashing the commits in this PR. I'd sat you can ignore this warning by now. BTW, please note that the PR still has conflicts.

[MisatoTremor]

@phansys Thank you for your answer.

I can't see any conflicts, though.

[phansys]

I can't see any conflicts, though.

Don't you see this message?

This branch cannot be rebased due to conflicts

[MisatoTremor]

Nope, I get this

[MisatoTremor]

I get conflicts when I try to rebase, though. Still weird, but I will do this now

[MisatoTremor]

OK done @phansys. Just wanted to ask if you could also check that I did not reintroduce some of the old issues?

[phansys]

OK done @phansys. Just wanted to ask if you could also check that I did not reintroduce some of the old issues?

I'm not aware about the related issues, as I'm not using this extension. If you have in mind some previous issues related to these changes, please try to add tests covering the issues with a clear reference to them.

[MisatoTremor]

@phansys I was just referring to the issues you pointed out in your reviews. To make sure I did not reintroduce one of them while rebasing.

[phansys]

@phansys I was just referring to the issues you pointed out in your reviews. To make sure I did not reintroduce one of them while rebasing.

Oh, don't worry about that then :+1:

[franmomu]

Hi, thanks for the PR!

Just a couple of comments, looking at the method calls ->getIdentifier(false, true) seems a little bit weird to me and the way the ids are flattened (return implode(' ', $id);) seems like it should belong somewhere else to me.

So what about making getIdentifier return always an array (deprecating passing true) like the one from ClassMetadata and also extracting the "flattener" to a class so it could be used somewhere else.

[MisatoTremor]

I'm open to that @franmomu of course. But it might be out of scope for this PR as it would need changes all over DoctrineExtensions. So maybe it warrants a prerequisite PR for itself.

But would leave that decision up to you maintainers before starting work on it.

[rotdrop]

So what about making getIdentifier return always an array (deprecating passing true) like the one from ClassMetadata and also extracting the "flattener" to a class so it could be used somewhere else.

The flattener should probably kept in sync with

https://github.com/doctrine/orm/blob/536b65f02b853cc9c8bfb184d23a4cc4feecf002/lib/Doctrine/ORM/Utility/IdentifierFlattener.php#L15-L20

which actually just does what Steffen does. Or perhaps use the IdentifierFlattener from ORM instead of coding the same thing again?

I'm still trying to find time to post a corresponding pull request for Translatable, and other things ... it would be nice, if this PR could finally be merged.

[MisatoTremor]

@rotdrop I remember using IdentifierFlattener during initial development of this, which did not work at the time, but that may have change since then ...

@phansys Could you please have another look at this? I am currently lost as how to prevent the deprecation reports by PHPUnit which lead to the failed tests. Also I have no idea why the PHPStan error in Geo.php suddenly appears.

[phansys]

I am currently lost as how to prevent the deprecation reports by PHPUnit which lead to the failed tests.

I guess you should add the new $flatten argument in the existing calls to WrapperInterface::getIdentifier().

Also I have no idea why the PHPStan error in Geo.php suddenly appears.

I'll try to take a look at this.

[phansys]

Also I have no idea why the PHPStan error in Geo.php suddenly appears.

Regarding this, I think we could safely remove that ignored error from the baseline (see https://phpstan.org/r/b92f2814-4530-4cda-aaf4-236c6c3eaadf).

https://github.com/doctrine-extensions/DoctrineExtensions/blob/118a001c1d0855df5c9ee35c1588d80a4d517d97/phpstan-baseline.neon#L808-L812

[MisatoTremor]

I guess you should add the new $flatten argument in the existing calls to WrapperInterface::getIdentifier().

The calls were not the problem, but the classes implementing the interface, only adding the second parameter there resolved the error. Changing the interface was also not necessary.

Regarding this, I think we could safely remove that ignored error from the baseline (see https://phpstan.org/r/b92f2814-4530-4cda-aaf4-236c6c3eaadf).

Done. Thank you!

[MisatoTremor]

@phansys And another error which I think is unrelated. My guess would it to be a hiccup on GitHubs end. Can you please re-run the failed jobs as I can't do that here. It ran successful local.

[phansys]

The issues reported by PHPStan should be resolved by #2475.