Follow-up for small things mentioned in #1481156-26: Incorrect logic in creating url to fetch information about project updates and #30.
Note the only two things remaining from that are to take out the reference to the issue 1481156 and make one of the in line comments a sentence.
Patch from spicerpa and chrisjlee moved over here. please include them in commit credit.
According to https://www.drupal.org/core/beta-changes
It qualifies for ok to work on during the beta because
This is not a feature.
It is an unfrozen change only: docs.
Re word the comments to be correct.
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | novice | Instructions |
No.
No.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 2374201.3.patch | 1.18 KB | yesct |
| 1481156-documentation-followup-33.patch | 2.6 KB | yesct |
Comments
Comment #2
yesct commentedThe spelling error, funcionality , was fixed in #2083943: Drupal core spelling minor fixes
does not apply because #2002116: Convert core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php to phpunit moved/removed
core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php
which also removed the => "Unit tests", quote problem.
#2002116: Convert core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php to phpunit fixed the inline comment needing to start with a space and end with a period:
- //first test that we didn't break the trivial case
it also fixed the space after // and capital in
//for enabled projects, adding the site key
but did not make it a sentence.
(That is now in core/modules/update/tests/Drupal/update/Tests/UpdateFetcherTest.php )
and
// http://drupal.org/node/1481156 test incorrect logic when URL contains
+ // a question mark.
is still in there. (also in UpdateFetcherTest)
So looks like we need to fix these last two things.
as chrisjlee mentioned, missing from the patch moved here was the fix webchick asked for
adding docs to the test class
#1594256: Convert update tests to PSR-0 moved UpdateCoreUnitTestCase from update.fetch.inc
to
core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php
and #1431634: Clean up API docs for update module added the docs (it also corrected one of the places the issue number 1431634 was mentioned directly.)
Comment #3
yesct commentedThese should be the only remaining cleanups that webchick asked for.
They are not *incorrect* comments, so I think that makes this a task (not a bug)
need to evaluate https://www.drupal.org/core/beta-changes
Comment #4
yesct commentedThe novice task on this one is to do a review. Please read the review instructions linked from the issue summary.
Check that the patch is not doing any out of scope or unrelated fixes.
See if the corrections look ... correct. You may need to read the tests to make sure the docs are correctly describing what the test is doing.
Comment #5
yesct commentedComment #6
Palashvijay4O commentedLooks good to me . :)
Comment #7
yesct commented@Palashvijay4O thanks for looking at this.
More info on how to do a review is at:
https://www.drupal.org/contributor-tasks/review
It helps if you make some comment about what you looked at and how you evaluated it.
If you feel like there is a problem, move it to "Needs work", and state what concerns you have.
If you feel like it is in scope, is a good approach to the solution, you have read all the lines of the patch, etc... please describe that kind of thing and mark it "RTBC".
If you did a partial review, say what parts you did, and what still needs to be done, and leave it at "Needs review".
Comment #8
Palashvijay4O commented1. There is now uniformity in the following lines describing what the tests are doing
// First test that we didn't break the trivial case.
// For disabled projects it shouldn't add the site key either.
// For enabled projects, test adding the site key.
2. also previously there was missing period
3. Now this line seems more clear to me and also means the same
4. And yeah it doesn't break anything that was fixed previously . :)
So RTBC !!
Comment #9
alexpottComment #10
alexpottThis issue is an unfrozen change (documentation) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed e27f007 and pushed to 8.0.x. Thanks!