D10. Check access by default is deprecated

The last submitted patch, 2: rules-drupal10-compatibility-3368140-2.patch, failed testing. View results →

These are two separate things and they need to be addressed by separate patches in separate issues. You may use this current issue for one of the problems then open up a new issue for the other.

Also note that the Rules version of autocomplete.js is forked from core, so any modifications than need to be made to this library should be the same as was done in core.

is this a clone of https://www.drupal.org/project/rules/issues/3257978 → ?

As I said above in #4, the patch tries to addressed two separate problems that should be handled in two separate issues.

Actually, I fixed all the JavaScript problems in 🐛 [10.0] Remove jQuery dependency from the once feature Fixed , including re-writing autocomplete.js like I suggested in #4.

The remaining issue is about accessCheck(). The issue summary and title for THIS issue should be changed to address the accessCheck() problem, and a patch that only fixes the accessCheck() problem should be posted here. That's what I was asking back in June. I will gladly commit that fix if someone does the work, otherwise you will have to just wait for me to get around to some of these things that don't affect me personally.

Hi TR,

Thanks for your notice. I reworked the patch, so now it fixes only the issue with accessCheck() function.

The last submitted patch, 7: rules-access-check-fix-3368140-7.patch, failed testing. View results →

hello _shY, i have change some code then fixed issue.
created patch please review.

The last submitted patch, 10: Check-access-by-default-is-deprecated-3368140.patch, failed testing. View results →

It should be TRUE, correct? That is the default if accessCheck() isn't used ...

Yes, you are right. TRUE is the default behavior.

But after this change, the test failed anyway.

The problem is that during test $storage->getQuery()->accessCheck() - returns NULL, but it should return the object itself, to be able to use other functions.

Still trying to figure it out.

When doing Unit testing, core Drupal services have to be simulated because they are not usable directly. So you can see in EntityFetchByFieldTest::testActionExecutionWithLimit() that we create a dummy entity storage and tell PHPUnit how that dummy entity storage should respond to a loadByProperties() call:

    // Create dummy entity storage object.
    $entity_storage = $this->prophesize(EntityStorageInterface::class);
    $entity_storage->loadByProperties([$field_name => $field_value])
      ->willReturn($entities)
      ->shouldBeCalledTimes(1);

This piece of code shows that we use the entity storage directly in the code being tested (the code being tested is EntityFetchByField::doExecute()), and that we use the loadByProperties() method directly in our testing. The test case is set up to contain entity data (defined in $entities) so we know what loadByProperties() should return. And we're specifying that we're expecting it to be called once and only once. This is a valid way to test things because were are NOT trying to test the storage class or the loadByProperties() method - those are defined by core Drupal and are tested by core Drupal. We just need to specify how that storage and that method behaves in our limited, well-defined use case.

Likewise, with the database query. The entity query service is not available, so we need to mock up how we expect it to behave and what we expect it to return under the conditions of our test. You will see this code in that same test case:

    // Create dummy query object.
    $query = $this->prophesize(QueryInterface::class);
    $query->condition($field_name, $field_value, '=')
      ->willReturn($query->reveal())
      ->shouldBeCalledTimes(1);
    $query->range(0, $limit)
      ->willReturn($query->reveal())
      ->shouldBeCalledTimes(1);
    $query->execute()
      ->willReturn($entity_ids)
      ->shouldBeCalledTimes(1);

This defines how the database query should operate and what it should return.

The reason the test fails is because when you modify the code to add the accessCheck(), you also need to modify the TEST in this location to know what to do when that method is called on the query. Remember, we're just using a simulated query here because this is Unit testing.

So what needs to be done here is to add something like:

    $query->accessCheck(TRUE)
      ->willReturn($query->reveal())
      ->shouldBeCalledTimes(1);

to the above code that creates the dummy query object. I'll leave that to you.

All this is only necessary for Unit testing, because Unit testing is a very minimal framework without all the overhead of building and loading a full Drupal site.

Does this all make sense? Rules has a lot of complicated test cases to ensure that changes to the code base don't break the functionality and to ensure that the functionality works as intended. But this means that when functionality is changed, like adding an accessCheck(), we need to change the test so that the test knows what we expect to happen.

Thanks, for the explanation.

It's become clear now. I tried to implement that, let's see if the test will pass.

TR, thank you again for your long explanation =)

It really helped. The tests are green.

Thank you. Committed.

Automatically closed - issue fixed for 2 weeks with no activity.