Problem/Motivation

It would be good to wrap this check into a service that can be extended, tested and overridden.

Proposed resolution

Write a service to handle this functionality.

Remaining tasks

Patch.

User interface changes

None.

API changes

Function is removed, service is added.

Data model changes

None.

CommentFileSizeAuthor
#3 2642142-checker-service.patch5.65 KBSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Assigned: Unassigned » Sam152
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Active » Needs review
FileSize
2642142-checker-service.patch5.65 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2642142-checker-service.patch, failed testing.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review

Setting back to needs review, the fails are an error with the bot.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Parent issue: » #2642154: [META] Reduce procedural code and write tests.
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Needs work 
  1. +++ b/src/ActivationCheck.php
@@ -0,0 +1,84 @@
+class ActivationCheck implements ActivationCheckInterface {

    Service can have some unit tests. 

  2. +++ b/src/ActivationCheck.php
@@ -0,0 +1,84 @@
+  protected $settings;

    This name is misleading imo should be config. settings mean something else in Drupal world. 

  3. +++ b/src/ActivationCheck.php
@@ -0,0 +1,84 @@
+    if (isset($_GET['colorbox']) && $_GET['colorbox'] == 'no') {

    Let's use $request here. 

  4. +++ b/src/ActivationCheck.php
@@ -0,0 +1,84 @@
+    $pages = Unicode::strtolower(_colorbox_array_to_string($this->settings->get('advanced.pages')));

    Can we convert this function to some static method?

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Service can have some unit tests.

There is a ticket to add tests, but I am focusing on getting everything out of the module file first.

This name is misleading imo should be config. settings mean something else in Drupal world.

Settings is the name of the config object. A new issue can be opened to clarify settings/config. This was basically meant to be a straight move into a service.

$this->settings = $config->get('colorbox.settings');

Let's use $request here.

Good idea.

Can we convert this function to some static method?

A later patch removes the need for this function.

frjo’s picture

frjo CreditAttribution: frjo commented

Committed to 8-dev, thanks!

frjo’s picture

frjo CreditAttribution: frjo commented
Status: Needs work » Fixed

  • frjo committed ff222a9 on 8.x-1.x authored by Sam152 
    Issue #2642142 by Sam152: Move the _colorbox_active check into a service

Status: Fixed » Closed (fixed)

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