Automated Drupal 10 compatibility fixes

Hi, there is any plan to merge it and create a new release compatible with D10?

I've tested the patch and scan by my own and found another compatibility, here my patch to turn the module Drupal 10 compatible.

Hi @icurk and @RokSiEu,

Sorry for tagged you but some of you can review this patch and release a new version D10 compatible.

Thanks for your time and help.

@MaxMendez for future reference typically you shouldn't RTBC your own patches, even core committers make sure someone else looks at code if they've touched it in any way other than to maybe add a period to a comment :)

That said, I've used this patch on a project, and it got unstuck so that's a great start. Here is a code review:

1. +++ b/context_groups.info.yml
   @@ -1,7 +1,7 @@
   +core_version_requirement: ^8.8 || ^9 || ^10
   

   Perhaps it's time to drop support for Drupal 8? Drupal 9 is EOL but it's useful to keep it for at least one version to provide an upgrade path.

2. +++ b/src/Form/GroupAddForm.php
   @@ -200,7 +200,7 @@ class GroupAddForm extends FormBase {
   +    $query = $this->entityTypeManager->getStorage('context')->getQuery()->accessCheck(FALSE);
   

   I'm not convinced the accessCheck should be skipped on this query. The groupExists method is a public method on a regular (read non-internal) class, and as much as it **really shouldn't** be, I think it could be used by others' custom code.

   I think the method should be made private, or the accessCheck invoked.

@darvanen: The ::groupExists() method is a callback for the machine name element of the form ( https://git.drupalcode.org/project/context_groups/-/blob/fc7d8209b00a851... ), and as such is more concerned with determining if a given name is unique on the given site. Uniqueness can't really be evaluated if some results are filtered from the query, so ::accessCheck(FALSE) probably makes the most sense; otherwise, it might be possible for different users who can't see each others contexts to bind the same name, and make a mess for _other_ users who can see both? Otherwise, it seems like we would have to get into some kind of namespacing to define "uniqueness" domains? Seems like something of a rather larger overhaul of the module than we're looking to do here.

As for the method's visibility, I've gotta admit that PHP's callable/callback-passing semantics are a little fuzzy to me: If it _was_ defined as protected/private, would it be usable as a callback for the machine name element? Looks like there's a call to the "exists" callback in MachineName::validateMachineName() ( https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... ), which means that it would be trying to call this method that it strictly should not be able to see, but that it was passed by something that _can_ see it? Will the callback expressed as an array of object and method, I'm not sure that visibility could correctly be resolved... maybe it would be ignored? Bit of naive testing indicates that it is not possible:

<?php
class A {
  public function methodOne() {
    echo "one... ";
    return [$this, 'methodTwo'];
  }

  protected function methodTwo() {
    echo "two... ";
  }
}

$instance = new A();

$callable = $instance->methodOne();
call_user_func($callable);

Resulting in:

one... 
Fatal error: Uncaught TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot access protected method A::methodTwo() in /home/user/scripts/code.php:16
Stack trace:
#0 {main}
  thrown in /home/user/scripts/code.php on line 16

With the newer first-class callback syntax introduced in PHP 8.1 (with $this->methodTwo(...) instead of [$this, 'methodTwo']), it _does_ appear to be possible; however, such seems to be getting rather outside of the intended scope of this issue.

@adam-vessey

Uniqueness can't really be evaluated if some results are filtered from the query

Very good point, bad idea to set the access check then.

As for the private/protected method, I love how you approached it. I'm afraid I don't know the answer to that. I'm quite new to this module (performing an upgrade) so I'm not familiar with where this form gets used, seems to be an AJAX call but I ran out of time to investigate further. I tried adding a breakpoint there but it wasn't triggered by the Add or Edit Context form submissions.

seems to be getting rather outside of the intended scope of this issue

Yeah maybe, I just wanted to raise it since the access checks are being introduced for very good reasons and I would prefer not to lose the security hardening opportunity that this represents. But simply adding ->accessCheck(FALSE) does preserve current functionality. I'm not going to kick up a fuss if that does go in.

Hi @darvanen, as you can see the change of status was a month later of the path, but no one cared about this module for month, but now when time is short it is easy come and talk about the work of others.

Why don´t drop D8 support?, the original didn't and was my mistake did not drop that moment. Be free to improve and create a pr or patch to help to get a new release soon.

Why the desition of accessCheck(FALSE)?
Well, really thanks @darvanen of time of review and testing in how the the module work and conclude what makes the most sense.

@MaxMendez I meant no disrespect, it’s just the process for getting work committed in core and most contributed modules.

If I adjust the code then I am no longer in a position to review it.

You are right, time is short, but I did not know about this module until a few days ago, I’m not responsible for the response time of the current maintainers, who are volunteering their time just like you and me 🙂

I’m afraid I don’t have all the answers regarding the access check. I don’t even fully understand what this module does yet. Did the missing access check cause an error? What were you doing when it did?