Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When reviewing http://drupal.org/project/project_dependency I found a bug #1241712: Entire module as patch comment-1 which was cloned from drupal_parse_dependency
When incrementing $major the $op was not ajusted.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal-dependency-1247476-26.patch | 9.45 KB | savkaviktor16@gmail.com |
#12 | drupal-dependency-1247476-12.patch | 9.68 KB | clemens.tolboom |
#9 | drupal-dependency-1247476-9.patch | 9.35 KB | clemens.tolboom |
#7 | drupal-dependency-1247476-7.patch | 8.76 KB | clemens.tolboom |
#4 | drupal-dependency-1247476-3.patch | 7.55 KB | clemens.tolboom |
Comments
Comment #1
clemens.tolboomThe 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.
Comment #2
clemens.tolboom(huh ... I definitely wanted a review)
Comment #4
clemens.tolboomDo we need this? It think this is better. Anyway patch #1 fails as $op is reduced from == to =
Test fails due to the change done in common.inc
19 days to next Drupal core point release.
Comment #5
clemens.tolboomI think this patch needs some more tests.
Ie validate the inline comments of
drupal_parse_dependency($dependency)
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedSince 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.
Comment #7
clemens.tolboom@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().
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedA 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.
Comment #9
clemens.tolboomThanks for the review (please use dreditor though :p)
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
Added whiteline before
Deleted
Added more test.
Fixed
Comment #10
clemens.tolboomThis needs a reroll as outlined by xjm on http://xjm.drupalgardens.com/blog/rerolling-drupal-8-core-patches
Comment #11
xjmThanks @clemens.tolboom! While you're at it, there's also a few things that need cleanup to standards:
This class needs a docblock, and no trailing inline comment.
This summary should start with a verb in the third person.
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.
This should begin "tests" and have commas and periods, at least.
Trailing whitespace here.
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]).
Needs a period, and "concat" is not actually a noun. :)
See the standards for documenting lists for how to document this parameter.
This return value needs a description.
These function summaries should be clarified a bit (and also begin with 3rd-person verb forms and end with a period).
These need documentation, as well as a blank line between the @param and @return sections. Also, the datatype should be lowercase (string).
Comment #12
clemens.tolboom@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.
Comment #13
clemens.tolboom[Powered by #1115636: Issue Macros and Templates]
This relates to #1013302: Versioned dependencies fail with dev versions and git clones
Comment #14
clemens.tolboom#12: drupal-dependency-1247476-12.patch queued for re-testing.
Comment #15
clemens.tolboomStill a bug in D8 ModuleHandler::parseDependency
This is true for Drupal 8.
Comment #17
Wim LeersIs this still relevant?
Comment #21
pingwin4egI 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.
Comment #22
anya_m CreditAttribution: anya_m as a volunteer and at Skilld commentedGoing to reroll
Comment #23
clemens.tolboom@pingwin4eg good change!
@anya_m great :-)
I myself face palms for the duration of this issue. But then again it's a subtle bug.
Comment #24
ashishdalviAssigning this issue for Drupal Mumbai Code Sprint Dec 2017
Comment #26
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedRe-rolled. Also, I've adjusted patch in keeping with a new structure and short array syntax.
Comment #28
clemens.tolboom@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?
Maybe add some documentation here? What about
// As we increment the major version we need to adjust the operator too
This TODO should be 'resolved' aka it must be added to the test then fixed if still applicable.
Weird wording mentioning 'fails'
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?
Comment #29
clemens.tolboom- Test has CR notice 'DRUPAL_CORE_COMPATIBILITY' https://www.drupal.org/node/2082661
Guess we need to move to 8.6.x-dev too.
Comment #30
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #38
quietone CreditAttribution: quietone at PreviousNext commentedModuleHandler::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!
Comment #39
pingwin4eg@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:
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.