This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.
Problem/Motivation
The following snippet is from template_preprocess_links()
// Add a "data-drupal-link-query" attribute to let the
// drupal.active-link library know the query in a standardized manner.
if (!empty($link['query'])) {
$query = $link['query'];
ksort($query);
$li_attributes['data-drupal-link-query'] = Json::encode($query);
}
Proposed resolution
Missing a use statement for Drupal\Component\Utility\Json
but as we are not getting a fail on testbot it appears we are also missing tests! The Drupal\system\Tests\Theme\FunctionsTest::linksTest()
needs to be improved to catch this.
Remaining tasks
- Write patch (novice)
- Review patch to check it fixes the issue, the change is properly documented and for coding standards. Provide test evidence (novice)
- Keep issue summary up to date (novice)
- reroll reroll instructions
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff-2217755-33-35.txt | 447 bytes | Les Lim |
#35 | core-2217755-35-nonexistent-json-class.patch | 4.19 KB | Les Lim |
#33 | core-2217755-33-nonexistent-json-class.patch | 4.2 KB | Les Lim |
#29 | drupal8-non-existent-class-json-2217755-29.patch | 4.18 KB | er.pushpinderrana |
#29 | interdiff-2217755-25-29.txt | 1.09 KB | er.pushpinderrana |
Comments
Comment #1
dawehnerThere is probably just a missing use statement, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis looks like a big hole in our test coverage?
Not sure how this is novice, except if we also identify which test case need to be extended.
Comment #3
alexpott@Damien Tournoud - yeah I debated this in my head when posting the issue - but it is a nice issue - fixing something broken and adding a test.
@dawehner - yep for some reason I missed that - thanks!
Comment #4
alexpottAdded location of test to improve to issue summary.
Comment #5
Alumei CreditAttribution: Alumei commentedUpdated the Test-Case.
Comment #6
Alumei CreditAttribution: Alumei commentedBoth updated test and fix.
Comment #7
Alumei CreditAttribution: Alumei commentedComment #9
dawehnerThis looks quite fine already!
Note: We could use \Drupal::urlGenerate()->generate and String::checkPlain instead
Comment #10
Alumei CreditAttribution: Alumei commentedNow uses String::checkPlain instead of check_plain.
Also removed 2 unused "Use" Statements.
About the use of \Drupal::urlGenerate()->generate I'm not sure. As the test currently explicitly differenciates between url based and rout based links. Though just for the added json testcase it makes sense. I'll change that.
Comment #11
Alumei CreditAttribution: Alumei commentedChanged the json-test part to be route-based and use Drupal::urlGenerate()->generate() instead of url().
Comment #12
Daniel Norton CreditAttribution: Daniel Norton commentedIf this issue isn't resolved in the next few days, it should be split into two issues.
1) The one-liner that actually fixes the bug that wasn't detected.
2) The more complex code that detects the bug.
Comment #13
Daniel Norton CreditAttribution: Daniel Norton commentedComment #14
Daniel Norton CreditAttribution: Daniel Norton commentedRe-rerolled for PSR-4
Comment #15
Daniel Norton CreditAttribution: Daniel Norton commentedComment #17
Daniel Norton CreditAttribution: Daniel Norton commented14: non-existent-class-json-2217755-14.patch queued for re-testing.
Comment #20
Daniel Norton CreditAttribution: Daniel Norton commentedTests pass locally. Testbot is not providing sufficient detail to identify the failure.
Comment #21
martin107 CreditAttribution: martin107 commentedTest information was inadequate :-(
1) More information was obtained by enabling better error reporting see
admin/config/development/logging
2) Web browser testing highlighted the need to keep up with recent HEAD changes to the location of Json::encode()
Comment #22
martin107 CreditAttribution: martin107 commentedComment #23
alexpottGiven that this a php bug.
Comment #24
martin107 CreditAttribution: martin107 commentedPatch no longer applies.
Comment #25
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedRerolled patch.
Comment #27
star-szrThe rerolled patch shrank by quite a bit, before reroll was 6.75 KB and now 3.36 KB. Can you try again @er.pushpinderrana?
Comment #28
er.pushpinderrana CreditAttribution: er.pushpinderrana commented@Cottser, I would definitely try again tomorrow, I am looking into cause of above test failure. I hope would be able to fixed this tomorrow.
Thanks!
Comment #29
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedOne more try with updated patch.
Comment #30
er.pushpinderrana CreditAttribution: er.pushpinderrana commented@Cottser, this time Test Bot make it green :). Nice.
Thanks!
Comment #31
alimac CreditAttribution: alimac commentedPatch does not apply to HEAD, tagging Needs reroll.
Comment #32
alimac CreditAttribution: alimac commentedHere are the reroll instructions.
Comment #33
Les LimPatch didn't apply because it tried to put a 'use' statement in a place where another 'use' statement has since been added. Rerolled.
Comment #34
Les LimAdding sprint tag.
Comment #35
Les LimNew patch to keep 'use' statements grouped nicely.
Comment #36
dawehnerWe seem to have proper test coverage now. I wonder whether we could somehow in the future use static code analyze to find
those kind of errors directly in the testbot.
Comment #37
alexpottCommitted 93beccc and pushed to 8.0.x. Thanks!