Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Status: Active » Closed (fixed)
FileSize
8.05 KB

The attached patch solves the issue but has some discussion points:

1. '==' seems not to be equal to '=' ... that is the $op : '==' is not transformed into $op : '='
The patch transforms a == into = ... my idea is that postprocessing does not have to test for both anymore.

2. the original_version gets prepended 1 space. Why? Why not two or more?
I would opt for the no space variant.

clemens.tolboom’s picture

Status: Closed (fixed) » Needs review

(huh ... I definitely wanted a review)

Status: Needs review » Needs work

The last submitted patch, drupal-dependency-1247476-1.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
+++ b/includes/common.inc
@@ -7220,6 +7220,7 @@ function drupal_parse_dependency($dependency) {
+        if ($op == '==') $op = '=';

Do we need this? It think this is better. Anyway patch #1 fails as $op is reduced from == to =

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,191 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+    $dependencies['foo (==1.2)']['result']['versions'][0]['op'] = '==';

Test fails due to the change done in common.inc

19 days to next Drupal core point release.

clemens.tolboom’s picture

Status: Needs review » Needs work

I think this patch needs some more tests.

Ie validate the inline comments of drupal_parse_dependency($dependency)

// Core version is always optional: 7.x-2.x and 2.x is treated the same.

// Drupal considers "2.x" to mean any version that begins with
// "2" (e.g. 2.0, 2.9 are all "2.x"). PHP's version_compare(),
// on the other hand, treats "x" as a string; so to
// version_compare(), "2.x" is considered less than 2.0. This
// means that >=2.x and <2.x are handled by version_compare()
// as we need, but > and <= are not.

Lars Toomre’s picture

Since there can be a number of comparison outcomes in drupal_parse_dependency(), I would suggest adding a unit test focused solely on this function exercising each one of the comparison operators with known good and bad values.

Edit: Also new Test class in the patch can be converted to unit test since it does not reference variable, globals or database. That will help performance longer-term once this gets in.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

@Lars Toomre: Thanks for the pointer to DrupalUnitTestCase (it's bloody fast) :-)

Patch now has
- simple version_compare tests.
- core version added as a test.

Testing for ie 2.9 equals to 2.x should be done by another UnitTest as both
- http://api.drupal.org/api/drupal/modules--system--system.install/functio...
- http://api.drupal.org/api/drupal/modules--system--system.admin.inc/funct...
both use drupal_check_dependencies().

Lars Toomre’s picture

A couple of thoughts...

For completeness, I think the comparison test cases should include (since minimal cycle cost):

2.1 < 2.9
2.9 < 2.10
2.1 < 2.10
2.10 < 2.11

Also curious about what would happen if contrib accidently used 2.01 instead of 2.1 in these string tests.

I also would remove the commented out print_r() line.

Finally, a phpdocblock nit... A blank line should be before each @return.

clemens.tolboom’s picture

Thanks for the review (please use dreditor though :p)

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,224 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+      '1:>' => array(

Added more < compare test.

Added 1 = test which suprisingly succeeded .. 2 == 2.0

The 2.01 == 2.1 fails as version_compare does a string compare on each part ... it is a developer err so I leave that test as comment for future reference

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,224 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * @return array

Added whiteline before

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,224 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+      //else {
+        //$this->assertTrue(TRUE, print_r($result, TRUE));
+      //}

Deleted

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,224 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+      '1:>' => array(

Added more test.

+++ b/modules/simpletest/tests/common.test
@@ -2154,6 +2154,224 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * @return array

Fixed

clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom
Status: Needs review » Needs work
xjm’s picture

Thanks @clemens.tolboom! While you're at it, there's also a few things that need cleanup to standards:

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+class DrupalDependenciesTestCase extends DrupalUnitTestCase { //WebTestCase {

This class needs a docblock, and no trailing inline comment.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+  /**
+   * Some tests to validate the comments from drupal_parse_dependency.
+   */

This summary should start with a verb in the third person.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+        array( '2.9', '2.10'), // 0 not trimmed ?
+        array( '2.10', '2.11'), // 10 < 11 ?
...
+        // array( '2.1', '2.01'), //TODO: this fails.
+        array( '2', '2.0'), // 2 equals 2.0 ... weird

Let's make these comments a little more to standard, and remove the TODO. It would be better to determine why it fails, or open a followup issue, etc. rather than adding the todo in the codebase.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * Test version patterns like ==1.4 <=1.4 or >1.x

This should begin "tests" and have commas and periods, at least.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * Tests are quite simple for normal release versions like 1.3
+   * but to compare against ie 1.x sometimes needs 2 ops.
+   * ¶
+   * @see drupal_parse_dependency() for detailed documentation

Trailing whitespace here.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+    // Include core version
...
+    // This should be the same as foo (1.2) apart from original_version
...
+    // This should be the same as foo (1.2) apart from original_version and op
...
+    // This should be the same as foo (1.2) apart from original_version and op
...
+    // This should be the same as foo (1.x) apart from original_version
...
+    // This should be the same as foo (1.x) apart from original_version
...
+    // This should lead to >= 2.x
...
+    // Note: Just one op with increased major
...
+    // This should lead to >=1.x and < 2.x
...
+    // Note: Just one op with increased major
...
+    // Major less then
...
+    // Major greater equal

All these comments need periods. Also, a lot of them are very vague and could do to be clarified. Finally, note that it is "less than" (vs. "less then" [sic]).

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * Makes an ordered string concat from given versions

Needs a period, and "concat" is not actually a noun. :)

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * @param array $versions
+   *   array('op' => '', 'version' => '')

See the standards for documenting lists for how to document this parameter.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * @return string

This return value needs a description.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * Simple helper
...
+   * Compare to versions arrays

These function summaries should be clarified a bit (and also begin with 3rd-person verb forms and end with a period).

+++ b/modules/simpletest/tests/common.testundefined
@@ -2154,6 +2154,238 @@ class DrupalSystemListingTestCase extends DrupalWebTestCase {
+   * @param String $op
+   * @param String $version
+   * @return array
...
+   * @param array $expected
+   * @param array $result
+   * @return boolean

These need documentation, as well as a blank line between the @param and @return sections. Also, the datatype should be lowercase (string).

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
9.68 KB

@xjm thanks for the feedback.

I rebased the patch from #9 and tried to apply the review from #11.

Not all is done as I'm puzzled mostly about explaining in plain English what happens.

clemens.tolboom’s picture

clemens.tolboom’s picture

#12: drupal-dependency-1247476-12.patch queued for re-testing.

clemens.tolboom’s picture

Assigned: clemens.tolboom » Unassigned
Status: Needs review » Needs work

Still a bug in D8 ModuleHandler::parseDependency

+++ b/core/includes/common.inc
@@ -7609,6 +7609,8 @@ function drupal_parse_dependency($dependency) {
           if ($op == '>' || $op == '<=') {
             $matches['major']++;
+            if ($op == '>') $op = '>=';
+            if ($op == '<=') $op = '<';

This is true for Drupal 8.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Issue summary: View changes

Is this still relevant?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pingwin4eg’s picture

Title: drupal_parse_dependency fails on major version ops (>7.x) and (<=8.x) » ModuleHandler::parseDependency fails on major version ops (>27.x) and (<=28.x)
Issue tags: +Needs reroll, +Quick fix

I didn't encountered this bug myself, I'm simply searching for another issue for the dependency parser. But from what I can see here, in patches, this particular issue was not fixed, so I suppose it is still relevant.

P.S.: Changing numbers in the title, since it's not about Drupal core only. That method also used for contrib modules, and numbers 7 & 8 are like they mean core.

anya_m’s picture

Going to reroll

clemens.tolboom’s picture

@pingwin4eg good change!

@anya_m great :-)

I myself face palms for the duration of this issue. But then again it's a subtle bug.

ashishdalvi’s picture

Issue tags: +DrupalMumbaiCodeSprint

Assigning this issue for Drupal Mumbai Code Sprint Dec 2017

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

savkaviktor16@gmail.com’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.45 KB

Re-rolled. Also, I've adjusted patch in keeping with a new structure and short array syntax.

Status: Needs review » Needs work

The last submitted patch, 26: drupal-dependency-1247476-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

clemens.tolboom’s picture

@Saviktor thanks for re rolling.

I wrote this code ages ago and not sure about wording, TODOs and function purpose. There are lots of comments but are those useful?

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -748,6 +748,8 @@ public static function parseDependency($dependency) {
    +              if ($op == '>') $op = '>=';
    +              if ($op == '<=') $op = '<';
                 }
    

    Maybe add some documentation here? What about

    // As we increment the major version we need to adjust the operator too

  2. +++ b/core/modules/system/src/Tests/Common/DrupalDependenciesTest.php
    @@ -0,0 +1,224 @@
    +        //TODO: Is the implicit space done by ModuleHandler::parseDependency() a problem?
    +        // Next test fail as I expected no space as part of original_version.
    +        // $full = "'" . $result['name'] . "' '" . $result['original_version'] . "'".
    

    This TODO should be 'resolved' aka it must be added to the test then fixed if still applicable.

  3. +++ b/core/modules/system/src/Tests/Common/DrupalDependenciesTest.php
    @@ -0,0 +1,224 @@
    +        // Adding spaces in the dependencies array fails.
    

    Weird wording mentioning 'fails'

  4. +++ b/core/modules/system/src/Tests/Common/DrupalDependenciesTest.php
    @@ -0,0 +1,224 @@
    +  /**
    +   * Compare to versions arrays
    +   *
    

    This text does not match/describe the code purpose I guess.

    All items from $expected MUST be in $result but not the other way around. $result may contain more items then $expected. Why?

clemens.tolboom’s picture

Version: 8.5.x-dev » 8.6.x-dev

- Test has CR notice 'DRUPAL_CORE_COMPATIBILITY' https://www.drupal.org/node/2082661

Guess we need to move to 8.6.x-dev too.

cilefen’s picture

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Version: 9.4.x-dev » 9.0.x-dev
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

ModuleHandler::parseDependency was removed in Drupal 9.0.0 in #3104306: Remove BC layers in the extension component, making this outdated.

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!

pingwin4eg’s picture

Version: 9.0.x-dev » 9.4.x-dev
Status: Closed (outdated) » Needs work

@quietone, please don't close issues just because corresponding class methods were removed.

ModuleHandler::parseDependency was removed, but its code was not. It is in the Drupal\Component\Version\Constraint::parseConstraint().

And the code block still like this:

          if ($op == '>' || $op == '<=') {
            $matches['major']++;
          }

So I'm sure the issue is not outdated/resolved. I don't have drupal projects ATM, so I can't run the code to check.

Someone please confirm the issue with dependencies parsing still exists.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.