Skip to content
Snippets Groups Projects

Issue #3501727: Remove the deprication notice, remove the entity type manager...

Closed Mariusz Andrzejewski requested to merge issue/drupal-3501727:3501727-try-to-simplify into 11.x

Issue #3501727: Remove the deprication notice, remove the entity type manager from AdminNegotiator and service definition, use the DeprecatedServicePropertyTrait to handle deprecation

Closes #3501727

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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 {
  • 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) {
  • 254 254 $this->assertSame($expected_queries, $recorded_queries);
    255 255 $expected = [
    256 256 'QueryCount' => 17,
    257 'CacheGetCount' => 82,
    257 'CacheGetCount' => 81,
  • 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.

    • Mariusz Andrzejewski changed this line in version 8 of the diff

      changed this line in version 8 of the diff

    • Ok, that makes sense, I was freaking out a bit because of this test - I couldn't understand why it started failing, I though it was related to that admin check, but as long as you say it is not (or should be handled as a part of the other issue) I reverted the change.

    • Please register or sign in to reply
  • added 1 commit

    • 32deaff2 - Issue #3501727: Revert cron UI test changes. Revert return type for applies...

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 17 commits

    Compare with previous version

  • added 67 commits

    Compare with previous version

  • added 1 commit

    • 600b7bde - Update the expected value for CacheGetCount in StandardPerformanceTest

    Compare with previous version

  • 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);
    • this looks good to me, I'd consider doing a \Drupal::service() then in this case like we do with missing arguments. we don't know for certain that the next parameter is what we want, especially in case make further changes to the constructor in the future.

    • 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.

    • Mariusz Andrzejewski changed this line in version 14 of the diff

      changed this line in version 14 of the diff

    • Hmm, this is a good argument, we don't have it covered in tests, so it's easy to overlook that. I've changed the implementation to statically call for the service.

    • Please register or sign in to reply
  • 335 335 $this->assertSame($expected_queries, $recorded_queries);
    336 336 $expected = [
    337 337 'QueryCount' => 17,
    338 'CacheGetCount' => 82,
    338 'CacheGetCount' => 81,
    • didn't expect that to be still here. fewer cache requests is always good, not 100% sure I understand why.

    • 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)

    • Thanks for investigating, it's a tiny optimisation but still nice.

    • Mariusz Andrzejewski changed this line in version 15 of the diff

      changed this line in version 15 of the diff

    • Please register or sign in to reply
  • added 1 commit

    • 2104c527 - Get AdminContext from container instead of relying on the constructor arguments

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading