Abstract the code from block visibility for php filter check to be a condition plugin. Separate this from the existing current path check.

Eclipse

Parent Issue:

#1743686: Condition Plugin System

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: EclipseGc » larowlan
larowlan’s picture

Status: Active » Needs review
FileSize
4.86 KB

Status: Needs review » Needs work

The last submitted patch, condition-php-1921546.2.patch, failed testing.

larowlan’s picture

Failed because testbot out of diskspace.

larowlan’s picture

Status: Needs work » Needs review

#2: condition-php-1921546.2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, condition-php-1921546.2.patch, failed testing.

larowlan’s picture

Still failing because out of diskspace

file_put_contents(): Only 0 of 84852 bytes written, possibly out of free disk space
podarok’s picture

Issue tags: +testbot

tag

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.91 KB
602 bytes

Re-roll to reference container namespaces.

Status: Needs review » Needs work

The last submitted patch, condition-php-1921546.9.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.91 KB

These pass locally, but merging HEAD anyway

larowlan’s picture

bump

EclipseGc’s picture

Issue tags: -testbot

#11: condition-php-1921546.11.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +testbot

The last submitted patch, condition-php-1921546.11.patch, failed testing.

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
4.75 KB

Ok, let's see how this one fairs.

tim.plunkett’s picture

Just some nitpicks, this looks good.

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+use Drupal\Core\Condition\ConditionManager;
...
+    $this->manager = new ConditionManager($this->container->getParameter('container.namespaces'));

Why not just
$this->manager = $this->container('plugin.manager.condition');

You're already using $this->container.

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+  public static $modules = array('system', 'php');

Should have a docblock, "Modules to enable." is the standard comment.

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+  /**
+   * Defines test information.
+   */
...
+  /**
+   * Sets up the tests.
+   */

No docblocks needed here. Yeah.

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+    // Grab the php condition and configure it to check against a php snippet.

s/php/PHP

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+    $this->assertTRUE($condition->execute(), 'PHP condition passes as expected.');

assertTrue

+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,75 @@
+    $this->assertEqual('When the given PHP evaluates as TRUE.', $condition->summary());
...
+    $this->assertEqual('When the given PHP evaluates as FALSE.', $condition->summary());
...
+    $this->assertEqual('No PHP code has been provided.', $condition->summary());

Just a matter of style, but I think its normally $this->assertEqual($variable, 'string');, so just switching them.

EclipseGc’s picture

FileSize
3.63 KB
5.47 KB

Alrighty then! Since conditions aren't actively used by anything, apparently they were not updated for the namespace stuff so I fixed that in this patch.

Eclipse

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The code and tests look good. Without a use case or UI, I can't judge it by any other criteria. So, RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

No tests..?

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests, -testbot
+++ b/core/modules/php/lib/Drupal/php/Tests/Condition/PhpConditionTest.phpundefined
@@ -0,0 +1,73 @@
+class PhpConditionTest extends DrupalUnitTestBase {

Test here!

webchick’s picture

LOL sorry about that! Chrome froze loading mid-patch and so I only saw the first two hunks. :)

I will have some D8 time Wednesday so will try and get to it then, if not before. Alex/catch/etc. feel free to beat me to it. :D

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll

error: patch failed: core/lib/Drupal/Core/CoreBundle.php:293
error: core/lib/Drupal/Core/CoreBundle.php: patch does not apply
EclipseGc’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

removed the core bundle stuff since that patch went in.

Eclipse

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Committed a015229 and pushed to 8.x. Thanks!

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

updated change notice at http://drupal.org/node/1961370

alexpott’s picture

Forgot to mention... during the commit I updated the documentation to the latest standards...

+++ b/core/modules/php/lib/Drupal/php/Plugin/Core/Condition/Php.phpundefined
@@ -0,0 +1,75 @@
+   * Overrides \Drupal\Core\Condition\ConditionPluginBase::buildForm().
...
+   * Overrides \Drupal\Core\Condition\ConditionPluginBase::submitForm().
...
+   * Implements \Drupal\Core\Executable\ExecutableInterface::summary().
...
+   * Implements \Drupal\condition\ConditionInterface::evaluate().

All updated to {@inheritdoc} ... see http://drupalcode.org/project/drupal.git/commit/a015229

Status: Fixed » Closed (fixed)

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