Query Form doesn't work

Hello,

Here is my review:

1. +++ b/src/Controller/CmisRepositoryController.php
   @@ -319,11 +319,12 @@ class CmisRepositoryController extends ControllerBase {
   +    $permissions = ['permission_callbacks'];
   

   'permission_callbacks' is not a permission. Also the changed code is the code executed as a permission callback.

2. +++ b/src/Controller/CmisRepositoryController.php
   @@ -319,11 +319,12 @@ class CmisRepositoryController extends ControllerBase {
   +    return AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');
   

   No need to put that here, instead of inside the if statement.

3. +++ b/src/Controller/CmisRepositoryController.php
   @@ -319,11 +319,12 @@ class CmisRepositoryController extends ControllerBase {
   +    //return AccessResult::neutral();
   

   No commented code.

I think you have not understood what needs to be done in this issue and how access callback works.

What needs to be done is:

• Create a dedicated permission to access the query page and change the routing.yml according to that
• In src/Form/CmisQueryForm.php, filter the connections select to only show the one accessible for the user

New permission added
Connection filtering on QueryForm

cmis-query-form-not-working-3167758-8.patch must be ignored
New patch available

New patch uploaded
cmis-api-query-form-doesnt-work-3167758-9.patch must be ignored

Hello,

Going into the right direction now!

But you don't take correctly the previous review points into account.

Here is my review:

1. +++ b/cmis.permissions.yml
   @@ -9,3 +9,7 @@ access all cmis browsers:
   +access cmis query page:
   

   Please put it above "permission_callbacks".

2. +++ b/cmis.permissions.yml
   @@ -9,3 +9,7 @@ access all cmis browsers:
   +  title: 'Access the CMIS query page'
   

   Access to the CMIS query page

3. +++ b/cmis.permissions.yml
   @@ -9,3 +9,7 @@ access all cmis browsers:
   +  description: 'Allow access to CMIS query page'
   

   Allow access to the CMIS query page.

4. +++ b/src/Form/CmisQueryForm.php
   @@ -45,6 +45,12 @@ class CmisQueryForm extends FormBase {
   +    $current_user = \Drupal::currentUser();
   

   You should do Dependency injection, but as you extends formBase, there is already an helper method currentUser()

5. +++ b/src/Form/CmisQueryForm.php
   @@ -45,6 +45,12 @@ class CmisQueryForm extends FormBase {
   +    foreach($configuration_options as $config_id => $config_name){
   +      if(!$current_user->hasPermission('access cmis browser ' . $config_id)){
   

   Coding standards issue.

6. +++ b/src/Form/CmisQueryForm.php
   @@ -45,6 +45,12 @@ class CmisQueryForm extends FormBase {
   +      if(!$current_user->hasPermission('access cmis browser ' . $config_id)){
   

   And what happens if the user has "access all cmis browsers" permission?

Coding standard fixed

Hello,

Thanks for the updated patch. Here is my review.

1. +++ b/src/Form/CmisQueryForm.php
   -- 
   Subject: [PATCH 2/2] Issue #3167758: coding standard fix
   

   When updating a patch, please rebase or squash your commits into one commit. Otherwise it is hard to review.

2. +++ b/cmis.permissions.yml
   @@ -7,9 +7,9 @@ access all cmis browsers:
   +  title: 'Access to the CMIS query page '
   

   Trailing space

3. +++ b/cmis.permissions.yml
   @@ -7,9 +7,9 @@ access all cmis browsers:
   +  description: 'Allow access to the CMIS query page'
   

   Still no dot at the end of the sentence.

4. +++ b/src/Form/CmisQueryForm.php
   @@ -45,10 +45,12 @@ class CmisQueryForm extends FormBase {
   +      $permission_all_access = 'access all cmis browsers';
   

   This variable does not depend of the foreach so declare it outside of it. And as it is not used elsewhere, you can avoid using a variable.

Hi,

Here is the new patch.

Hello,

Thanks for the new patch. Code is ok for me now.

I will apply the patch to test it.

Tested ok.

Merged on 8.x-2.x, 2 commits because I forgot to take a file in the commit.

And merged on 3.0.x too.