Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Turbo] Fix broadcast with composite primary key #1857

Open
wants to merge 10 commits into
base: 2.x
Choose a base branch
from

Conversation

chapterjason
Copy link

@chapterjason chapterjason commented May 19, 2024

Q A
Bug fix? yes
New feature? no
Issues Fix #1856
License MIT

Kind of a feature as it adds support for using broadcast with entities using a composite primary key.

  • Add tests to reproduce the issue
  • Fix composite primary keys
  • Split IdAccessor into another DoctrineIdAccessor to prevent duplicated code
  • Introduce IdFormatter to format the values returned from the doctrine function getIdentifierValues
  • Introduce DoctrineClassResolver to resolve real entity class names. (Doctrine;Proxy)

Others

  • Add missing dash in the CONTRIBUTING.md
  • Fix primary key type in test data (Song.php)

Last workflow issue with phpstan is fixed in #1854

Fixed more than I wanted, a bit unsure about the injection of services into Broadcaster in the Mercure integration, was even confused that the turbo services for the Mercure integration are registered in the MercureBundle.

@chapterjason chapterjason marked this pull request as ready for review May 19, 2024 16:02
@carsonbot carsonbot added Bug Bug Fix Status: Needs Review Needs to be reviewed labels May 19, 2024
@kbond kbond requested a review from dunglas May 22, 2024 14:46
@kbond kbond changed the title Fix broadcast with composite primary key [Turbo] Fix broadcast with composite primary key May 22, 2024
@kbond kbond added the Turbo label May 22, 2024
Comment on lines +135 to +137
if (!isset($this->broadcastedClasses[$entityClass])) {
$this->broadcastedClasses[$entityClass] = [];
$r = new \ReflectionClass($entityClass);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is worth the change.

if ('createdEntities' !== $property) {
$id = $em->getClassMetadata($class)->getIdentifierValues($entity);
$id = $this->doctrineIdAccessor->getEntityId($entity, $em);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you illustrate precisely what cannot be done before, what it would look like, and same after your changes ? :)

Comment on lines +37 to +39
if (!$this->doctrine) {
return $class;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if $em is passed ?

Comment on lines +22 to +27
private $doctrine;

public function __construct(?ManagerRegistry $doctrine = null)
{
$this->doctrine = $doctrine;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ppp there please

Comment on lines +48 to +54
$values = $em->getClassMetadata($entity::class)->getIdentifierValues($entity);

foreach ($values as $key => $value) {
if (\is_object($value)) {
$values[$key] = $this->getIdentifierValues($em, $value);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some doc/example here ?

Comment on lines +50 to +66
$container->addCompilerPass(new class() implements CompilerPassInterface {
public function process(ContainerBuilder $container): void
{
$serviceIds = [
...$container->findTaggedServiceIds('turbo.broadcaster'),
...$container->findTaggedServiceIds('turbo.renderer.stream_listen'),
];

foreach ($serviceIds as $serviceId => $tags) {
$definition = $container->getDefinition($serviceId);

$definition
->addArgument(new Reference('turbo.id_formatter'))
->addArgument(new Reference('turbo.doctrine_class_resolver'));
}
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tags refer to BroadcasterInterface and TurboStreamListenRendererInterface services, so you cannot add those arguments there as it would potentially impact other services which don't use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Status: Needs Review Needs to be reviewed Turbo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Turbo] Doesn't work with entity that uses composite primary key
4 participants