All entity handlers are instantiated per entity type, and they get the entity type definition passed in by EntityTypeManager:
public function createHandlerInstance($class, EntityTypeInterface $definition = NULL) {
if (is_subclass_of($class, 'Drupal\Core\Entity\EntityHandlerInterface')) {
$handler = $class::createInstance($this->container, $definition);
}
else {
$handler = new $class($definition);
}
However, the two route provider handlers in core choose to ignore this:
public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
return new static(
$container->get('entity_type.manager'),
$container->get('entity_field.manager')
);
}
Instead, they expect the entity type to be passed in all over again:
public function getRoutes(EntityTypeInterface $entity_type) {
This is confusing, and pointless, and could lead to bugs if developers see this method and assume that one route provider object can handle any entity type.
Let's deprecate the parameter, and have the route providers use the $entity_type they are given on construction.
Deprecate the `$entity_type` parameter of
`EntityRouteProviderInterface::getRoutes()` and have route providers read the
entity type they are given on construction. Make the parameter optional and add
a BC layer for providers that still declare it as required. Remove the parameter
in drupal:13.0.0.
- [x] Write the patch
- [x] Create change record
- [ ] Review
None.
- The `$entity_type` parameter of `EntityRouteProviderInterface::getRoutes()`
is deprecated in drupal:11.4.0 and removed in drupal:13.0.0.
- Implementing `getRoutes()` with a **required** `$entity_type` parameter is
deprecated; the parameter must be optional.
- Route providers now read the entity type injected on construction.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #13
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #14
joachim commentedYup it's still there.
Comment #17
skifdesu commentedAdded a merge request deprecating entity_type in the route provider.
Comment #18
joachim commentedI don't think the new parameter to __construct() needs BC handling, because the class has a create(), and so any creation must go through that. But will ask to check.
Comment #19
smustgrave commentedNot 100% sure either but think we should use constructor promotion if we are touching these files. Would be in scope to fix other parameters on these files too, least that's what we have done in the past. Think this will need a CR as well.
Also summary appears to not be using the template so tagged for that.
Thanks.
Comment #20
acbramley commentedWe also need to solve BC, see MR comment and also even with those fixes you will need to add a constructor to any route providers that don't have one (i.e the same changes as the RevisionHtmlRouteProvider
As for the constructor parameter (question in #18), I don't think it needs to be nullable, EntityTypeManager::createHandlerInstance always injects it
Comment #21
acbramley commentedRemoving novice tag based on the BC complexity
Comment #22
skifdesu commentedComment #23
dcam commentedThank you for your work on this issue, but it isn't ready for review yet. The issue is tagged as needing an issue summary update and a change record. The summary should be updated to include the full template. All deprecation message URLs in the merge request need to be updated to the URL of the change record.
Comment #24
skifdesu commentedUpdated the issue summary and created the change record.
Comment #25
smustgrave commentedBelieve we missed the 11.4 boat so will have to be bumped to 11.5 please.
Comment #26
quietone commentedThe title reads as if this is a just deprecation of an unused variable, so changing title to reflect the larger change here.
Comment #27
skifdesu commented