[D8] Group SSO

Automated Review

Best practice issues identified by drupalcs.

FILE: ...james/drupal/drupal8/web/modules/custom/gsso/src/Form/GSSOSettingsForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
10 | WARNING | The class short comment should describe what the class does and
| | not simply repeat the class name
--------------------------------------------------------------------------------

FILE: ...ames/drupal/drupal8/web/modules/custom/gsso/src/SSOTypePluginInterface.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
10 | WARNING | The class short comment should describe what the class does and
| | not simply repeat the class name
--------------------------------------------------------------------------------

FILE: ...ome/james/drupal/drupal8/web/modules/custom/gsso/src/SSOTypePluginBase.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
10 | WARNING | The class short comment should describe what the class does and
| | not simply repeat the class name
--------------------------------------------------------------------------------

FILE: /home/james/drupal/drupal8/web/modules/custom/gsso/src/Services/GSSO.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES
--------------------------------------------------------------------------------
9 | ERROR | [x] Non-namespaced classes/interfaces/traits should not be
| | referenced with use statements
15 | WARNING | [ ] The class short comment should describe what the class
| | does and not simply repeat the class name
155 | ERROR | [ ] Unnecessarily gendered language in a comment
202 | ERROR | [ ] Unnecessarily gendered language in a comment
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Manual Review

Git clone Url

The provided Git clone Url does not appear to follow the standards for Drupal projects. Please check.

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code. No third party assets included with module.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. Docblock documentation is missing, and there is no documentation on Drupal.org.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. I did not detect any obvious security weaknesses in the code.

Coding style & Drupal API usage

List of identified issues in no particular order:

1. Function gsso_help will return invalid HTML markup on route help.page.gsso as the closing </ul> tag is missing.
2. Function gsso_simplesaml-auth_user_attributes: The second "empty($claims_changes)" expression in the test "if (empty($claims_changes) || (!empty($claims_changes) && $claims !== $fss))" can be removed as the expression is always false by the time it is evaluated.
3. Abstract class AbstractGSSOSettingsForm appears to be missing a call to its parent constructor.
4. Class GSSO.php methods setUserClaims, removerUsermemberships, and associateuserToGroups are each missing an argument tryp declaration.
5. Class GSSO.php should declare the properties "logger" and "groupMembershipLoader" before using them in the constructor.
6. Class CrudFormTest typo in word "accesible'.
7. Class GSSOSettingsForm.php typo in word "unvalid".
8. Class names should follow the Drupal coding standards to use CamelCase, which has not the been done with classes like GSSO and GSSOSettingsForm. They should be Gsso and GssoSettingsForm to comply.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

@-nrzr- Please ignore my comment about the git url. I have found that the d.o site was previously providing a standard git url of

git clone https://git.drupalcode.org/project/[project_name]

, but now appears to provide a url of the form

git clone https://git.drupalproject.org:project/[project_name]

.

Drupal.org repository URL is always https://git.drupalcode.org/project/[project-name].git. It doesn't seem drupalproject.org is a domain registered for Drupal.org sites.

My mistake. The second example url should have been

git clone https://git.drupalcode.org:project/[project_name]

The colon separator now appears to be the standard.

When I run git clone https://git.drupalcode.org:project/drupal.git or git clone https://git.drupalcode.org:project/drupal, I get fatal: unable to access 'https://git.drupalcode.org:project/drupal.git/': URL using bad/illegal format or missing URL in the first case, and fatal: unable to access 'https://git.drupalcode.org:project/drupal/': URL using bad/illegal format or missing URL in the second case.

Only git clone https://git.drupalcode.org/project/drupal (with a warning about a redirect to https://git.drupalcode.org/project/drupal.git) and git clone https://git.drupalcode.org/project/drupal.git work.

I think I understand the issue here.

When you look at the version control tab for a project you own (or have maintenance rights over) then the git clone url contains a colon (":") separator. However, looking at this tab for other projects gives a url with a slash ("/") separator.

I am not sure of the reason for this - maybe someone from the D.O support team can advise?

For maintainers, the command to clone a repository is git clone git@git.drupal.org:project/<project_name>.git. The repository URL is git@git.drupal.org:project/<project_name>.git, not https://git.drupalcode.org:project/<project_name>.git.

@-nrzr-: I note that there have not been any changes to the module since it was originally reviewed, so the issues listed there, except for the comment about the git url still apply.

To clarify, the point 1 under "Coding style & Drupal API usage" should state that the "</ul>" closing tag is missing from the markup returned from hook_help().

It would be good for you to correct the issues identified.

Cheers.

• What follows is a quick review of the project; it doesn't mean to be complete
• Some of the points could be the same reported from the previous review
• For every point, I didn't make a complete list of the lines that should be changed, but I made an example of what needs to be changed
• The review points report what should be changed to follow the Drupal coding standards, correctly use the Drupal API, or avoid any (possible) security issue; if a review point isn't about one of those topics, the point makes that clear

/**
 * Class GSSOSettingsForm.
 */
abstract class AbstractGSSOSettingsForm extends ConfigFormBase {

  /**
   * Config settings.
   *
   * @var string
   */
  const SETTINGS = 'gsso.settings';

  /**
   * Provides an interface for an entity field manager.
   *
   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
   */
  protected $entityFieldManager;

  /**
   * Provides an interface for entity type managers.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * GSSO Form constructor.
   *
   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
   *   Provides an interface for an entity field manager.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   Provides an interface for entity type managers.
   */
  public function __construct(EntityFieldManagerInterface $entity_field_manager, EntityTypeManagerInterface $entity_type_manager) {
    $this->entityFieldManager = $entity_field_manager;
    $this->entityTypeManager = $entity_type_manager;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('entity_field.manager'),
      $container->get('entity_type.manager')
    );
  }

The name of an abstract class doesn't start with Abstract. Following the Drupal coding standards, the correct name would be GssoSettingsForm.

The documentation comment for the class should say something more than Class GSSOSettingsForm. as that information is already given from the class code.

A class that extends ConfigFormBase should use code similar to the following one. The difference is in the injected dependencies.

  /**
   * MediaSettingsForm constructor.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory service.
   * @param \Drupal\media\IFrameUrlHelper $iframe_url_helper
   *   The iFrame URL helper service.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   */
  public function __construct(ConfigFactoryInterface $config_factory, IFrameUrlHelper $iframe_url_helper, EntityTypeManagerInterface $entity_type_manager) {
    parent::__construct($config_factory);
    $this->iFrameUrlHelper = $iframe_url_helper;
    $this->entityTypeManager = $entity_type_manager;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
    $container->get('config.factory'),
    $container->get('media.oembed.iframe_url_helper'),
    $container->get('entity_type.manager'));
  }

$config = $this->configFactory->getEditable(static::SETTINGS);

$this->config(static::SETTINGS) returns an editable configuration object. There is no need to use the configuration factory.

  /**
   * Remove user from all his groups.
   *
   * @param \Drupal\user\UserInterface $account
   *   Account to remove from groups.
   * @param bool $debug
   *   Show debug values.
   */
  public function removeUserMemberships(UserInterface $account = NULL, $debug = FALSE) {
    // …
  }

It's not a security issue, nor a wrong use of Drupal API, but the method is expecting an account object that isn't null. A null account object would mean no task to be done, but also a Fatal error: Uncaught Error: Call to a member function on null error.

  /**
   * {@inheritdoc}
   */
  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
    // …
  }

The documentation comment for the class constructor is never {@inheritdoc}.

dependencies:
  - drupal:group

The Group module isn't a Drupal core module. The correct namespace for that module is group:group.

/**
 * Implements hook_uninstall().
 */
function gsso_uninstall() {
  \Drupal::configFactory()->getEditable('gsso.settings')->delete();
}

Drupal core modules don't remove their configuration values when they are uninstalled. I take that is done from Drupal core when the module is uninstalled.

I am closing this application, since there haven't been replies in more than eight months and the application was open more than twelve months ago.

Feel free to re-open it, once the project has been changed basing on what reported in the last review.