Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Add a non-existing module as a dependency of, for example, the blog module, load the module page, here is what you get:
Comment | File | Size | Author |
---|---|---|---|
#13 | 423664_dependency_fix_with_test_4.patch | 4.18 KB | Berdir |
#9 | 423664_dependency_fix_with_test_3.patch | 6.23 KB | Berdir |
#6 | 423664_dependency_fix_with_test_2.patch | 4.22 KB | Berdir |
#4 | 423664_dependency_fix_with_test.patch | 3.04 KB | Berdir |
#3 | 423664_dependency_fix_with_test.patch | 3.04 KB | Berdir |
Comments
Comment #1
BerdirThis is easy to fix, the only question is where.
As far as I understand it, the graph is a generic implementation, which doesn't care if a module exists or not. Because of that, I added a check in _module_build_dependencies() which simply ignores data for a module that doesn't exist.
We should probably add a test for this, but I'm not sure what exactly should be tested.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks like the good place to fix this.
Two things remain:
- could you please fix the whitespace issue, while you are at it
- we need a test for that ;)
Comment #3
BerdirUpdates:
- added missing whitespace
- Added a very basic test with a test module that has a dependency on a missing module. For some reason, the module tests are splitted up between system.test and module.test but as the ModuleDependencyTestCase was in system.module, I added my function to that one.
Comment #4
BerdirDouble post, ignore that.
Comment #5
chx CreditAttribution: chx commentedSorry -- but this needs a fix in graph.inc. The DFS can't add nodes / vertices to the graph. I would have sworn this was fixed there but then graph.inc saw many a change before getting in so we might have overlooked it.
Comment #6
BerdirOk, I tried to fix it directly in DFS :)
I had to make 2 changes, see attached patch:
- Only visit existing "sub-vertexes" (vertexes which are listed in path)
- Only add reverse_path information to existing vertexes
I don't think there is a easier way, because we want missing path/dependencies to be listed but not the reverse.
Comment #7
chx CreditAttribution: chx commentedI like it, if the bot likes it too then i will mark this as RTBC
Comment #9
BerdirGiven that DFS should not create missing vertexes, the tests actually test a wrong behavior. Updated the test...
Comment #10
BerdirComment #11
chx CreditAttribution: chx commentedThat makes sense. Some dimwit wrote those tests (aka me).
Comment #12
Dries CreditAttribution: Dries commentedWith the additional test in graph.test, it looks like we might not need the extra module test?
Comment #13
BerdirI assume that is true, especially as the module tests needs a test module just for a single check.
- Removed module test and module_test files
- Added a "." to the comment of the new assertion in graph.test
Comment #14
chx CreditAttribution: chx commentedNever would occur me to ask for less tests but I guess, yeah, that was superflous.
Comment #15
webchickCool. I tried applying only the test hunk and it found 2 failures, and dependencies[] = banana returns a whole crap load of errors. With the patch everything gets much better.
I have no bloody clue what's going on in those tests, but I assume they make sense to someone who understands graph.inc. :P
Added a period to
// Only visit existing vertexes
and changed "vertexes" to "vertices", made doubly sure I took out dependencies[] = banana ;) and committed to HEAD. Thanks!