Issue #3501727: Remove the deprication notice, remove the entity type manager...
Merge request reports
Activity
added 1 commit
added 1 commit
added 1 commit
added 1 commit
added 1 commit
55 47 * The route admin context to determine whether the route is an admin one. 56 48 */ 57 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, AdminContext $admin_context) { 49 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, AdminContext $admin_context) { 58 50 $this->user = $user; 59 51 $this->configFactory = $config_factory; 60 $this->entityTypeManager = $entity_type_manager; 61 52 $this->adminContext = $admin_context; 62 53 } 63 54 64 55 /** 65 56 * {@inheritdoc} 66 57 */ 67 public function applies(RouteMatchInterface $route_match) { 68 return ($this->entityTypeManager->hasHandler('user_role', 'storage') && $this->user->hasPermission('view the administration theme') && $this->adminContext->isAdminRoute($route_match->getRouteObject())); 58 public function applies(RouteMatchInterface $route_match): bool { changed this line in version 8 of the diff
49 43 * The current user. 50 44 * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory 51 45 * The config factory. 52 * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager 53 * The entity type manager. 54 46 * @param \Drupal\Core\Routing\AdminContext $admin_context 55 47 * The route admin context to determine whether the route is an admin one. 56 48 */ 57 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, AdminContext $admin_context) { 49 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, AdminContext $admin_context) { If we're going to provide bc for the property, we also need to provide bc for the constructor - because it's in the middle it'll need a type check.
Having said that though, this is a tagged service that we'd never expect anyone to interact with directly or subclass, so I think we could also consider grepping contrib for usages, and if there's none, just don't bother with bc here at all - constructor bc is 'best effort' but tends to be for plugins and similar that contrib/custom code is more likely to subclass.
there is in fact one: https://git.drupalcode.org/project/og_sm/-/blob/8.x-1.x/src/Theme/AdminNegotiator.php?ref_type=heads
I provided an example on how to do this in comment #6: https://www.drupal.org/project/drupal/issues/3501727#comment-15958184
Edited by Sascha Grossenbacherchanged this line in version 8 of the diff
254 254 $this->assertSame($expected_queries, $recorded_queries); 255 255 $expected = [ 256 256 'QueryCount' => 17, 257 'CacheGetCount' => 82, 257 'CacheGetCount' => 81, I actually didn't expect cache get counts to go down with this, interesting. I guess that's due to the access policy cache then, because permissions are loaded without the user role load? but I'd still expect something to load the entity type
lots of overlapping issues currently that I'm working on.
that's "only" a bootstrap cache, so fast chained and doesn't actually save a database query. the cache tag however might. https://www.drupal.org/project/drupal/issues/3500739 will add a bit more visibility into what exactly is happening.
https://www.drupal.org/project/drupal/issues/3500683 will then undo this, as it removes persistent caching in favor of this always loading the role.
and the cache tag cost should then be improved again with https://www.drupal.org/project/drupal/issues/3436146, which will preload common cache tags including entity_types, so it will be loaded together with a bunch of other cache tags.
changed this line in version 12 of the diff
- Resolved by Sascha Grossenbacher
118 118 * Make sure the cron UI reads from the state storage. 119 119 */ 120 120 public function testCronUI(): void { 121 $admin_user = $this->drupalCreateUser(['administer site configuration']); 121 $admin_user = $this->drupalCreateUser(admin: TRUE); the test doesn't actually need access to the admin theme, tests don't have that unless explicitly set up.
I've seen this fail a few times randomly recently, I think it was just that, if you revert this it should still work (most of the time anyway).
We should open an issue about it. it is related to the cron changes. but it's not related to this issue.
changed this line in version 8 of the diff
added 1 commit
added 1 commit
added 1 commit
added 17 commits
-
3b861479...e2b97e56 - 6 commits from branch
project:11.x
- cfa47da6 - 1 earlier commit
- 0c62c5de - Issue #3501727: Remove the deprication notice, remove the entity type manager...
- 9d4ae86d - Issue #3501727: Fix the tests for AdminNegotiator
- 238cb16f - Issue #3501727: Fix typo in group name
- 2a0182c5 - Issue #3501727: Rollback the phpunit.xml file.
- 270244da - Issue #3501727: Fix the performance tests cache counting.
- fd0b1637 - Issue #3501727: Fix the performance tests cache counting.
- 8f91af6b - Issue #3501727: Create admin user to test cron UI instead of relying on permissions
- 8cac9187 - Issue #3501727: Revert cron UI test changes. Revert return type for applies...
- c59939d4 - Issue #3501727: Improve the deprecation message
- ce3a0cd6 - Issue #3501727: Add link to issue in deprecation message.
Toggle commit list-
3b861479...e2b97e56 - 6 commits from branch
added 67 commits
-
ce3a0cd6...daf3cd72 - 54 commits from branch
project:11.x
- daf3cd72...8247260d - 3 earlier commits
- 2e0dd1e3 - Issue #3501727: Fix typo in group name
- 468d5bc2 - Issue #3501727: Rollback the phpunit.xml file.
- babf8a57 - Issue #3501727: Fix the performance tests cache counting.
- 34dfef33 - Issue #3501727: Fix the performance tests cache counting.
- 17b3452a - Issue #3501727: Create admin user to test cron UI instead of relying on permissions
- 32deaff2 - Issue #3501727: Revert cron UI test changes. Revert return type for applies...
- 39993e89 - Issue #3501727: Improve the deprecation message
- 3b861479 - Issue #3501727: Add link to issue in deprecation message.
- 9df47941 - Merge branch '11.x' of https://git.drupalcode.org/project/drupal into 3501727-try-to-simplify
- 9f5c562f - Merge branch '3501727-try-to-simplify' of git.drupal.org:issue/drupal-3501727...
Toggle commit list-
ce3a0cd6...daf3cd72 - 54 commits from branch
added 1 commit
- 600b7bde - Update the expected value for CacheGetCount in StandardPerformanceTest
53 * The entity type manager. 54 * @param \Drupal\Core\Routing\AdminContext $admin_context 52 * @param \Drupal\Core\Routing\AdminContext|EntityTypeManagerInterface $admin_context 55 53 * The route admin context to determine whether the route is an admin one. 56 54 */ 57 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, AdminContext $admin_context) { 55 public function __construct(AccountInterface $user, ConfigFactoryInterface $config_factory, AdminContext|EntityTypeManagerInterface $admin_context) { 58 56 $this->user = $user; 59 57 $this->configFactory = $config_factory; 60 $this->entityTypeManager = $entity_type_manager; 61 $this->adminContext = $admin_context; 58 59 if ($admin_context instanceof EntityTypeManagerInterface) { 60 $deprecated_service_name = EntityTypeManagerInterface::class; 61 @trigger_error("Passing the $deprecated_service_name (entity_type.manager service) to AdminNegotiator is deprecated in drupal:11.2.0 and will be removed in drupal:12.0.0. There is no replacement for this service, as it is not used. See https://www.drupal.org/project/drupal/issues/3501727", E_USER_DEPRECATED); 62 $this->adminContext = func_get_arg(3); On the other hand, we rely on what comes from the default implementation, argument 4 was required to be in the given type. This means that we should rely on the typing I suppose, if it's something else then this is the developer's error (and the exception should raised as usual) because as for now the required type for the 4th parameter was AdminContext and nothing else. And as you see the override only takes place when the arguments are passed in a deprecated order (the third argument is the entity type manager). It's not a big deal to use Drupal::service() but on the other hand, I don't see any reason to do that (other than just because it was this way in other places).
the reason to do that is to be future proof. we don't have test coverage for this BC layer, which is OK: but if a future update will add a new 4th argument that is in fact something else, then there is a good chance that we will forget to update this and would need to account for the 4th argument possibly being something unexpected as well.
As you said, it's not supposed to be called, so not a DI/performance concern.
changed this line in version 14 of the diff
335 335 $this->assertSame($expected_queries, $recorded_queries); 336 336 $expected = [ 337 337 'QueryCount' => 17, 338 'CacheGetCount' => 82, 338 'CacheGetCount' => 81, I couldn't let this sit, so I had to investigate.
The part that wasn't clear to me is that the access policy processor is only invoked if there is a hasPermission() check, but that never happens during submission of the login form and redirect, so we actually avoid loading the user role in this case.
It's not related to removing the hasHandler() check, it's because the hasPermission() and admin route calls are switched.
Yep, I think I already wrote this down in the issue. We immediately check the routing if it's not the admin route then the permission is not checked (hence the cache is not used, because there is no need to fetch any permissions data). So IMO it works correctly and it was because of the change in the order of checks
😄 (route first, permissions second)changed this line in version 15 of the diff
added 1 commit
- 2104c527 - Get AdminContext from container instead of relying on the constructor arguments