Improved recursion handling in EagerLoadingExtension

[carlobeltrame]

Q	A
Branch?	main for features
Tickets	#2444, #1959, CC @soyuka @teohhanhui
License	MIT
Doc PR	None, since this is more or less a refactoring, with only very technical changes

This PR works towards #2444. I refactored the EagerLoadingExtension a bit. Namely I changed 3 things:

• Avoid eager joining back to the parent that we just came from, for ToOne relations: When eager joining author.books, the nested joinRelations(..., Book::class, ...) call will ignore the book.author relation, because we already know the author.
• Remove unnecessary duplicate select statement: In Invalid DQL when using selfreferenced entity #1959 it was noticed that this exact select statement is already added a few lines above. The only exception is when fetchPartial is active, and in that case the current implementation was wrong anyways, because it always adds the full select. The fix from Fix #1959 do not join same association twice #1961 is really not needed for any conceivable case. So to clean up, I removed it.
• Avoid joining unnecessary recursive relations: This is the only commit that required adjusting the tests. The ultimate goal was: If a relation is readableLink: true, it should be eager loaded, including it's transitive relations (which are readable etc.). Otherwise, if the relation is at least readable: true, it should still be eager loaded (for generating the IRI), but not its relations. E.g. an author has many books and each book has many chapters. When eager loading the author's relations, we want to eager load the books as soon as author.books is readable, but unless author.books is also readableLink, we don't need to eager load book.chapters. This PR introduces the ability to addSelect('books') without calling the recursive $this->joinRelations(..., 'books', ...).

Since the implementation of the joinRelations method takes into account a lot of options, I visualized the behaviour here. Left is before this PR, right is after. Green is the first commit, red the second, blue the third, where I basically only moved the readableLink check later. This move resulted in some more (correct) joins in some Behat tests.

_{editor link for later reference}

These changes all don't break backward compatibility because of Doctrine's awesome ability to fetch missing relations when they are accessed.

Motivation

This PR is motivated by our application which uses the following pattern for serialization groups:

#[ApiResource(
  normalization_context: ['groups' => ['read']],
  denormalization_context: ['groups' => ['write']],
)]
class Book {
  #[Groups(['read', 'write'])]
  public string $title = '';

  /*
   * @ORM\ManyToOne(targetEntity="Person", inversedBy="books")
   */
  #[ApiProperty(readableLink: false, writableLink: false)]
  #[Groups(['read', 'write'])]
  public ?Person $author = null;
}

#[ApiResource(
  normalization_context: ['groups' => ['read']],
  denormalization_context: ['groups' => ['write']],
)]
class Person {
  #[Groups(['read', 'write'])]
  public string $name = '';

  /*
   * @ORM\OneToMany(targetEntity="Book", mappedBy="author")
   */
  #[ApiProperty(readableLink: false, writableLink: false)]
  #[Groups(['read', 'write'])]
  public Collection $books;
}

The idea is that we always want to present full representations in the API, never partial. And we always want to list all relations as links, except in a few cases where we embed them (using a quite elegant mechanism also based on serialization groups).

Unfortunately, with these groups, the current implementation of the EagerLoadingExtension will eager join absolutely everything, running into the join limit very quickly.

[soyuka]

I like this, please rebase against main if we can merge this it'd be a must

[carlobeltrame]

@soyuka rebased onto main. We also had to add another commit to make sure each alias is added to the select clause only once. Doctrine would cause nasty errors otherwise.

[carlobeltrame]

Not sure why the Behat Rector installation fails. But given that currently all PRs as well as the main branch have the same error, and I didn't touch anything composer.json-related, I think there is nothing to fix in this PR.

[alanpoulain]

You're right. I think @soyuka would like to review your PR before merging it. Nice work.

[soyuka]

Thank you very much for the work @carlobeltrame !