We need to respect versioned dependencies.

http://drupal.org/node/542202

CommentFileSizeAuthor
#13 project_dependency.versioned_dependencies_1351744_13.patch9.18 KBtrobey
#10 project_dependency.versioned_dependencies_1351744_10.patch6.16 KBrfay
#10 versioned_dependencies_interdiff_10_vs_9.txt3.26 KBrfay
#9 versioned_dependencies-1351744-9.patch5.69 KBtrobey

Comments

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Category: bug » feature
Priority: Normal » Major

Definitely we should get this in soon.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Title: Respect versioned dependencies » Respect versioned dependencies and soft dependencies
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

One major problem with versioned dependencies is that without git_deploy, the module versions won't be respected on the testbot (which uses git to check out). Are we going to always require git_deploy? I think that will be required.

The second issue is that requiring a dev is pretty iffy in Drupal right now. Which dev?

trobey’s picture

trobey commented

I have posted a proposal to allow dependency overrides on #1347790: Specify dependency overrides on the Automated Testing tab, comment #3.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Priority: Major » Critical

This is the only way we're going to be able to solve the knottier dependency issues.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Another look at this one says it's not too terribly difficult to do: At least the versioned dependencies aren't too bad. The parsing of >3.5, etc. might be ugly. But just getting versioned would be good.

  • Parse the major version out od the dependency. Note that it's 1.x, not 7.x-1.x
  • Add a new column in project_dependency_dependency for the major or other version
  • Dependency calculation will have to use the major.
          $recommended_release = project_release_get_current_recommended($dependent_pid, $api_term->tid);

    Note that project_release_get_recommended has an optional arg for the major.

Note that this will probably not solve every problem, as there are always some edge cases. I suspect it will solve #1802156: Rebuild dependencies for Media 7.x-2.x, but you can see in that one the complexity involved: file_entity was once a part of media but is no longer. So we have dependencies for both of them, and both might match with the same major version.

I'm ok with punting on the complicated cases like dependencies[] = exampleapi (>1.0, <=3.2, !=3.0) at this point.

Related: #1247516: drupal_parse_dependency fails on major version ops (>7.x) and (<=8.x) (project_dependency version)

Thanks so much for taking a look at this, @trobey.

trobey’s picture

trobey commented
Assigned: Unassigned » trobey
Category: feature » bug

I have some initial results. First, Drupal 7 has two functions that are used to parse versioned dependencies. The first function is drupal_parse_dependency() and the second one is drupal_check_incompatibility(). The second function calls the PHP function version_compare(). I have backported these two functions. This makes the whole thing a bit easier and we need to use the same approach or there will be discrepancies. And when this project is ported to Drupal 7 it will hopefully be a simple switch to the Drupal 7 functions.

But the problem is also worse than I initially thought. As I went through this I began to suspect versioned dependencies broke project_dependency and I have now confirmed it. The example is a dependency string of "media (<1.99)." No possible releases are found that match this string as I suspected. When I use a dependency string of "media" I find 20 possible releases in my test environment. So using versioned dependencies breaks project dependency and this is bug (and a feature request). So I am changing the category and assigning the problem to myself. I should have a patch shortly.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Even if we only had
(2.x)
it would solve most of the practical problems that we run up against.

I guess I agree this is a bug... but it's been a longstanding feature request :-)

trobey’s picture

trobey commented
Status: Active » Needs review
StatusFileSize
new versioned_dependencies-1351744-9.patch5.69 KB

Here is the patch. I had to add an argument to project_dependency_parse_dependency because projects can be from different core vesions of Drupal. This code looks like:

$dependency_component = $component_info['dependency'];
+ $parsed_component =
+ project_dependency_parse_dependency($dependency_component, $api_term);
+ $dependency_component = $parsed_component['name'];

This fixes the problem with introducing the versioned dependencies into Drupal 7. Each possible release is then checked using:

+ $check = project_dependency_check_incompatility($parsed_component,
+ substr($dependent_release['version'], drupal_strlen($api_term)+1));

and only compatible releases are included.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
StatusFileSize
new versioned_dependencies_interdiff_10_vs_9.txt3.26 KB
new project_dependency.versioned_dependencies_1351744_10.patch6.16 KB

Nicely, elegantly done, thanks!

I found one major issue - the handling of core version wasn't working because of use of drupal_strlen($api_term) instead of $api_term->name, etc. I redid that part to be a bit more explicit.

One nit in a misspelled function name.

I tested this with the junk_test_project .info file in a number of forms.

+++ b/project_dependency.drupal.incundefined
@@ -439,12 +439,26 @@ function project_dependency_get_external_component_dependencies($release_nid, $d
+        $check = project_dependency_check_incompatility($parsed_component,
+          substr($dependent_release['version'], drupal_strlen($api_term)+1));

Needs to use $api_term->name for removal. But I redid this to handle major version etc.

+++ b/project_dependency.moduleundefined
@@ -179,3 +179,100 @@ function project_dependency_get_external_release_dependencies($depending_release
+function project_dependency_check_incompatility($v, $current_version) {

Irrelevant typo, but corrected.

trobey’s picture

trobey commented

Looks great except for one problem. Did you know that someone had already backported drupal_parse_dependency and called it project_dependency_info_parse_dependency? It is almost exactly like the one that I wrote including the additional argument for the core version. It does not appear to be used anywhere. And it mentions project_dependency_info_check_incompatibility but I cannot find that function. As far as I can tell project_dependency_info_parse_dependency is never used. I would suggest that it be deleted.

I tested your changes in my local environment and they work fine.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

We should clean that up then. This has a long history of boombatower doing it... then it getting orphaned... being partially deployed on d.o... then me taking it on without a full understanding of his work, etc.

trobey’s picture

trobey commented
StatusFileSize
new project_dependency.versioned_dependencies_1351744_13.patch9.18 KB

Patch updated with the removal of the orphaned function.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs review » Fixed

Outstanding. Works for me. I did find one unrelated bug in testing #1817828: If no release is found for a dependency, an empty record/dependency array is created, I'll follow with that.

Committed: http://drupalcode.org/project/project_dependency.git/commitdiff/a6700556b

I'd love to get the Test Dependencies one in, and we can make a release and deploy. #698932: "test_dependencies" for dependencies / integration tests

I don't think there are any particular risks of this one, as it seems it would only affect projects using versioned dependencies.

Status: Fixed » Closed (fixed)

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

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Closed (fixed) » Patch (to be ported)

Deployed.
@trobey++

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Patch (to be ported) » Fixed

Painful D7 port in http://drupalcode.org/project/project_dependency.git/commitdiff/2c838b2c333

Hopefully that will do it.

Status: Fixed » Closed (fixed)

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