If a project has an ambiguous, un-namespaced dependency specified in one of it's components info file, and a component of that same name exists within the project, then project dependency should assume that the author intended the dependency to be local to the project.
The example issue is with the media component in the media project, specifically release 7.x-1.5. It has a dependency on 'file_entity': http://cgit.drupalcode.org/media/tree/media.info?h=7.x-1.5. The 7.x-1.5 release of media *also* contains a 'file_entity' component: http://cgit.drupalcode.org/media/tree/file_entity?h=7.x-1.5, so the assumption should be made that 7.x-1.5 version of media has a local dependency on its own file_entity module.
However, currently it is resolving to the file_entity component of the file_entity project: https://www.drupal.org/project/media/releases/7.x-1.5 (shows file_entity 7.x-2.0-beta3), which is not the intended outcome.
The following patch addresses this by adding an additional release_node parameter to project_dependency_guess_project, and gets the component list for that release and checks if the specified dependency matches anything on that list. If it matches, it assumes that the project is the local project, and not an external dependency.
Comment | File | Size | Author |
---|---|---|---|
#25 | fix_project_namespace_for_media-2796557-25.patch | 6.1 KB | trobey |
#24 | fix_project_namespace_for_media-2796557-24.patch | 4.03 KB | trobey |
#21 | 2796557.diff | 5.1 KB | drumm |
#20 | fix_project_namespace_for_media-2796557-20.patch | 3.22 KB | trobey |
#2 | 2796557-2.patch | 2.18 KB | Mixologic |
Comments
Comment #2
MixologicHere's the goods.
Comment #3
MixologicI built a query to show how much of an impact this would have, and while its relatively minimal (only 15 projects total are affected by the bad guesses), one of those is Drupal itself, which is why we get the "link module" dependency on the lower right of drupal release pages: https://www.drupal.org/project/drupal/releases/8.2.0-beta3.
For the curious, the query is this:
The following 15 projects will be helped by this patch:
Comment #4
trobey CreditAttribution: trobey as a volunteer commentedProject Dependency should not have to guess at projects. I think someone should file issues on each of these projects and ask them to specify their dependencies in an unambiguous manner.
Comment #5
trobey CreditAttribution: trobey as a volunteer commentedaws_glacier
There are no releases listed for this project. There is a 7.x development branch. Here are the dependencies:
dependencies[] = entity
dependencies[] = awssdk2
dependencies[] = views
dependencies[] = awssdk2:aws_glacier_fields
So this patch will not help this project.
Comment #6
trobey CreditAttribution: trobey as a volunteer commentedbootstrap_core
Only has a development release for Drupal 7. Three commits and nothing in over a year. It is true that bootstrap_field is specifying a dependency on bootstrap_ux and this is picking up bootstrap_ux project which is obsolete. But this is consistent with selecting projects with the same name as the module name, Since bootstrap_core does not have any formal releases I am not sure it makes any sense to worry about it.
Comment #7
trobey CreditAttribution: trobey as a volunteer commentedcodebook_print_pdf
This looks like it has been generated by Features. The alpha 2 release only has one component so it does not look like this patch will help it. In any case it only has an alpha release for alpha 2 and looks like it is only used on 2 websites.
Comment #8
trobey CreditAttribution: trobey as a volunteer commentedcod_support
All the modules for this start with the cod_ prefix. I do not see any dependencies starting with cod_ so this patch will not help.
Comment #9
Mixologicmostly this list was not to show what needed to be fixed, but how much of an impact this change would have, which is minimal.
Many of these projects are fixing old releases, which is data that is fed into project composer, so looking at the most recent release might not reveal what its fixing.
Comment #10
trobey CreditAttribution: trobey as a volunteer commentedPatch created for Drupal 8 core #2798891: Define project dependencies in core module .info files.
Comment #11
trobey CreditAttribution: trobey as a volunteer commentedFixed for Lingotek 8.x-1.x-dev #2798235: Include project namespace for module dependencies.
Comment #12
trobey CreditAttribution: trobey as a volunteer commentedERPAL Invoice has an Drupal 7 alpha release. It has two components erpal_invoice and erpal_invoice_ui. erpal_invoice_ui is dependent on erpal_invoice but this is working correctly:
Comment #13
trobey CreditAttribution: trobey as a volunteer commentedfacebook_status has no recommended releases. Looking at the releases they are for Drupal 6 except there is this:
facebook_status 7.x-1.x-dev
Release notes
This is a placeholder branch. The D7 branch of FBSS actually exists at the Statuses namespace.
So there is nothing to fix.
Comment #14
trobey CreditAttribution: trobey as a volunteer commentedMerci (8.x-2.x-dev) does have a dependency on office_hours which is picking up the office_hours project instead of the internal office_hours module. But some of the info.yml are still in the Drupal 7 info file format so it is difficult to file a patch for this.
Comment #15
trobey CreditAttribution: trobey as a volunteer commentedOpen Atrium (oa_core and oa_events) look okay. oa_events only has the oa_events component so there are no internal components. oa_core has a lot of components but they all are prefixed with oa_. The only project dependency prefixed that way is oa_subspaces and there is no internal module by that name. So I do not see any problem for these two.
Comment #16
trobey CreditAttribution: trobey as a volunteer commentedOpenCalais only has one component so this does not seem to apply.
Comment #17
trobey CreditAttribution: trobey as a volunteer commentedSlick is getting the right projects so this does not seem to apply but I filed an issue anyway #2799617: Include project namespace for module dependencies.
Comment #18
trobey CreditAttribution: trobey as a volunteer commentedMedia for Drupal 8 is not at drupal.org. The dependencies for Media 7.x-2.x-dev look okay.
Comment #19
MixologicThe purpose of this patch is not to fix any outstanding non-namespaced modules. The purpose is defensive in nature. The next time that a module either splits into multiple projects, or is absorbed by a master project, if those dependencies are not namespaced (which they are not guaranteed to be) project_dependency will guess the wrong thing.
The media module is the reason this issue came to light, is because site builders need to use *older* releases when working with the composer facade, or they may not want to wait until the next full release is rolled or use a dev release every time a problem comes up.
Since we do not have the ability fix the namespaces for the 7.x-1.5 release of media, the only way that we can get the proper dependency data for that particular release is to improve the guessing.
Comment #20
trobey CreditAttribution: trobey as a volunteer commentedI do not think that this patch will work because Media 7.x-1.5 already has been processed and had a project namespace added:
There are two problems here. The first one is that the project namespace has to be fixed. The second is a bit harder to understand. Since Media 7.x-1.5 has a dependency on itself (media_internet depends on media) it is picking up Media 7.x-2.0-beta2. The attached patch fixes both problems. Here are the results:
The patch is consistent with using project namespaces. I have not fixed Media 7.x-2.0-beta2. Should I fix that one too?
Comment #21
drummI tried out #20, but the
file_entity:file_entity
returned when runningdrush -v pdpp media
I took a different approach with this patch:
project_dependency_info_batch_process_release()
, if an un-namespaced dependency matches a component name, use the project’s namespace.external
to support namespaces.project_dependency_find_external_dependencies()
, addpdd.external = 1
to the query so it only returns external dependencies.Comment #22
trobey CreditAttribution: trobey as a volunteer commentedAfter you applied the patch did it do a database update? I think not since the number for the update hook has to be incremented to 7103 since other patches have been committed. I have updated code on my computer but I have not had enough time to update the patch.
I will have to take a closer look at your patch but the problem is that the project namespace is added at the time of the project release so changing the code will not fix something already released (which is what we are doing). The only way to fix this is to fix the database. Well, I guess there may be another way. It may be possible to use one of the Project Dependency drush commands to force the release to be reprocessed after the code is fixed.
Comment #23
drummTrue, I didn’t actually run the update. I want to be sure this fixes everything on reparsing. Hard-coding Drupal.org’s dependency IDs in an update should be a last resort.
I tried to test again with renumbering, but we don’t have a 5505675 in that table since dependencies have been reparsed. We actually have
media:file_entity
stored already, probably thanks to having #2 deployed.If I keep both #2 and #20 applied, this does seem to fully fix the problem (side effect of the fresh dev site, forgot to remove it). If I remove #2 and reparse, the extra dependencies come back. If I manually do what the update function tries to do, the problem is fixed again. And again, when I reparse the dependencies reappear.
That’s why I broadened the scope of my patch to fix the parsing step. Expanding on my explanations from #21:
This looked like a general bug.
!in_array($new_dependency, $components_in_release)
was assuming$new_dependency
wasn’t namespaced.This is the biggest behavior change in my mind, its what I think needs the most review. If I’m understanding the code correctly, it is adjusting the query to do what the function name says it will, but maybe there’s a reason for the current behavior I’m not aware of.
Comment #24
trobey CreditAttribution: trobey as a volunteer commented@drumm: That was useful. I checked and the project release was not detecting that the component was part of the project. I updated the patch. We do not want to just find external dependencies. This was one of the first bugs that I fixed in Project Dependency. If a project is dependent on the uc_attribute module of Ubercart which is dependent on the uc_product module of the Ubercart project which is dependent on ctools then we have to include internal dependencies in order to get the ctools dependency.
I have not checked the $new_dependency problem yet. I will try to take a look at that.
Comment #25
trobey CreditAttribution: trobey as a volunteer commentedI fixed $new_dependency and updated the patch.
Comment #26
drummThis looks good now. Testing with media 7.x-1.5 behaves as it should, it only depends on ctools.
Comment #27
drumm#25 has been deployed on www.drupal.org, I’ve rebuilt media and refreshed the Composer endpoint for the project.
Comment #29
trobey CreditAttribution: trobey as a volunteer commented