Incorrect logic in creating url to fetch information about project updates

we need to test this before.

Just 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.

I'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

$project['info']['project status url'] = 'http://www.example.com/?test=1';
$project['name'] = 'update_test';
$project['project_type'] = '';
$site_key = 'site_key';

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?

I'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.

Ok, 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.

re rolling the patch including both the fix and the test.

Patch from #9 worked just fine with the latest 8.x-dev
Seems pretty much straightforward

also, 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?

sorry, changed status unintentionally. Marking as RTBC as per #17

Good catch. Committed to 8.x. Moving to 7.x. Thanks!

marking as Patch (to be ported) for D7, patch #1 should do and the tests should backport easily from #9.

here are the patches, the first is the test only and the second is the backport of the patch.

Oops! the second patch again:

No difference from D8, so RTBC

Committed 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:

+++ b/modules/update/update.testundefined
@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
+class UpdateCoreUnitTestCase extends DrupalUnitTestCase {

This needs a line of PHPDoc to explain what it's doing.

+++ b/modules/update/update.testundefined
@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
+      'name' => "Unit tests",

Single quotes, since we're not interpolating any variables here.

+++ b/modules/update/update.testundefined
@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
+   * Tests _update_build_fetch_url according to issue 1481156

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.

+++ b/modules/update/update.testundefined
@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
+    //first test that we didn't break the trivial case
...
+    //For disabled projects it shouldn't add the site key either.
...
+    //for enabled projects, adding the site key
...
+    // http://drupal.org/node/1481156 test incorrect logic when url contains
+    // a question mark.

Comments should follow this format:

// Space before full sentence, starts with capital letter and ends in period,
// wraps at 80 characters.

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.

Hi, 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?

#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...

+++ b/modules/update/update.testundefined
@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
+
+class UpdateCoreUnitTestCase extends DrupalUnitTestCase {
+

Does this need a doc block explaining this class?

Oops, 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.

Changes 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:

@@ -697,3 +697,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
   }
 
 }
+
+class UpdateCoreUnitTestCase extends DrupalUnitTestCase {
+

HEAD:

/**
 * Tests update functionality unrelated to the database.
 */
class UpdateCoreUnitTest extends UnitTestBase {

#33: 1481156-documentation-followup-33.patch queued for re-testing.

We 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