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.
Update checks fail for modules that have a defined 'project status url' in their .info file containing a question mark. This is caused by the _update_build_fetch_url function in update.fetch.inc incorrectly inserting a question mark instead of an ampersand when adding get parameters to a url. The current code incorrectly uses the 'strpos' php function to check for an existing question mark in the url.
$url .= (strpos($url, '?') === TRUE) ? '&' : '?';
strops() will never return TRUE; if the 'needle' was found in the 'haystack' then its position (integer) would be returned.
Comments
Comment #1
RadioActiv CreditAttribution: RadioActiv commentedComment #2
RadioActiv CreditAttribution: RadioActiv commentedComment #3
RadioActiv CreditAttribution: RadioActiv commentedComment #4
filijonka CreditAttribution: filijonka commentedwe need to test this before.
Comment #5
lucascaro CreditAttribution: lucascaro commentedJust tried to apply this to D8 and it won't work because:
error: modules/update/update.fetch.inc: No such file or directory
re-created to work against D8.
Tested using junk_test_module and manually changing the "project status url".
It breaks before patching, It works after patching.
The patch in #1 should work for D7 (the only change was the path to the update module).
Edit, just tested the patch #1 agains D7 and it works.
Comment #6
lucascaro CreditAttribution: lucascaro commentedI'm writing tests for this issue and I think the problem might be deeper than we think.
If we in fact are supporting urls with a ? in them for "project status url" what is the desired behavior of _update_build_fetch_url?
let's say we have
then the function (with the patch) will generate the url:
http://www.example.com/?test=1/update_test/8.x&site_key=site_key
is that correct?
are we looking for something like:
http://www.example.com/update_test/8.x?test=1&site_key=site_key
or the first url should work?
Comment #7
lucascaro CreditAttribution: lucascaro commentedI'll just assume that the url needs to be of the form http://www.example.com/?project= so we can append the project name. let me know if I'm wrong.
Comment #8
lucascaro CreditAttribution: lucascaro commentedOk, here is the test case and the patch that solves it.
Please take time to review the test case as well since it's my first for core. I will gladly accept any suggestions you have to improve it.
The patch is the same as #5 re rolled to be tested with the new test.
Comment #9
lucascaro CreditAttribution: lucascaro commentedre rolling the patch including both the fix and the test.
Comment #10
RadioActiv CreditAttribution: RadioActiv commentedTo clarify what caused me to find the bug in the first place; I had a site that couldn't use clean urls that was acting going to act as a 'Feature Server' (fserver module).
The "project status url" ended up being something like
http://example.com/drupal_sites/feature_server/index.php?q=fserver
This caused the url generated by _update_build_fetch_url to be
http://example.com/drupal_sites/feature_server/index.php?q=fserver/MODULE_NAME/6.x?site_key=key&version=6.x-1.x
Since there were two question marks in the url returned, update checks always fail.
Comment #11
RadioActiv CreditAttribution: RadioActiv commentedPatch from #1 backported to D6. Adding tag for backport to D6.
Comment #13
RadioActiv CreditAttribution: RadioActiv commentedUpdate of patch for D6.
Comment #14
RadioActiv CreditAttribution: RadioActiv commentedComment #16
RadioActiv CreditAttribution: RadioActiv commentedRe-uploading D6 patch with 'do-not-test', since it doesn't apply to 8.x codebase.
Comment #17
valthebaldPatch from #9 worked just fine with the latest 8.x-dev
Seems pretty much straightforward
Comment #18
lucascaro CreditAttribution: lucascaro commentedalso, as @timmillwood made me notice, the D6 and D7 patches are missing the tests. Since it's a unit test they might be ported from the patch #9?
Comment #19
lucascaro CreditAttribution: lucascaro commentedsorry, changed status unintentionally. Marking as RTBC as per #17
Comment #20
Dries CreditAttribution: Dries commentedGood catch. Committed to 8.x. Moving to 7.x. Thanks!
Comment #21
lucascaro CreditAttribution: lucascaro commentedmarking as Patch (to be ported) for D7, patch #1 should do and the tests should backport easily from #9.
Comment #22
lucascaro CreditAttribution: lucascaro commentedhere are the patches, the first is the test only and the second is the backport of the patch.
Comment #24
lucascaro CreditAttribution: lucascaro commentedOops! the second patch again:
Comment #25
valthebaldNo difference from D8, so RTBC
Comment #26
webchickCommitted and pushed to 7.x for parity with 8.x, thanks! This is a good catch, and I love that it comes with a test!
However, there are a few things that could use some clean-up, so bumping back to 8.x and "needs work" for that. Trying to be thorough so you learn best practices. :)
Specifically:
This needs a line of PHPDoc to explain what it's doing.
Single quotes, since we're not interpolating any variables here.
Typically, we don't put issue IDs and the like in comments, because you can find that information via git blame. This should instead be changed to a description that would make sense to someone who'd never seen this issue before.
Comments should follow this format:
On the last one, once again don't bother referencing the issue node ID. Describe the functionality to someone who is coming across this test 6 months from now.
Comment #27
jurgenhaasHi, I'm a newbe to core contribution and just joined in on the core sprint at DrupalCon Munich. Picked this ticket as my first issue and wanted to cleanup the code but unfortunately can't find the file modules/update/update.test which is referred to in the patch from #24. To top part of the patch is already rolled into the latest D8 dev release but I wonder how to go about with the second part.
Can someone please point me in the right direction?
Comment #28
taubeta CreditAttribution: taubeta commented#22: update-custom_url_parse_D7-1481156-22.patch queued for re-testing.
Sorry, ignore that. I just wanted to see what happens by clicking re-test...
Comment #29
spicerpa CreditAttribution: spicerpa commentedComment #30
spicerpa CreditAttribution: spicerpa commentedDoes this need a doc block explaining this class?
Comment #32
xjmOops, sorry for the confusion. This patch has already been committed to D8 and D7, so the task now is to make a new patch against D8 with the changes described in #26. #30 is also correct.
Comment #33
chrisjlee CreditAttribution: chrisjlee commentedChanges are followup work as recommended by #26.
Also, No changes are needed for the class docblocks. They're actually already been committed to core:
Patch:
HEAD:
Comment #34
YesCT CreditAttribution: YesCT commented#33: 1481156-documentation-followup-33.patch queued for re-testing.
Comment #37
YesCT CreditAttribution: YesCT commentedWe are not doing followups in the same issues anymore, so we need a new issue. here it is: #2374201: Docs and quote coding standard follow-up from: Incorrect logic in creating url to fetch information about project updates