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

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

There is probably just a missing use statement, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

Damien Tournoud’s picture

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

alexpott’s picture

Issue summary: View changes

@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!

alexpott’s picture

Issue summary: View changes

Added location of test to improve to issue summary.

Alumei’s picture

Updated the Test-Case.

Alumei’s picture

Both updated test and fix.

Alumei’s picture

Status: Active » Needs review

The last submitted patch, 5: non-existent-class-json-2217755-5.patch, failed testing.

dawehner’s picture

This looks quite fine already!

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php
@@ -201,6 +209,8 @@ function testLinks() {
+    $expected_links .= '<li class="query-test"><a href="' . url('query/test', array('query' => $query)) . '">' . check_plain('Link with query') . '</a></li>';

@@ -237,6 +247,8 @@ function testLinks() {
+    $expected_links .= '<li class="query-test"><a href="' . url('query/test', array('query' => $query)) . '">' . check_plain('Link with query') . '</a></li>';

@@ -250,6 +262,9 @@ function testLinks() {
+    $encoded_query = check_plain(Json::encode($query));
+    $expected_links .= '<li class="query-test" data-drupal-link-query="'.$encoded_query.'" data-drupal-link-system-path="query/test"><a href="' . url('query/test', array('query' => $query)) . '" data-drupal-link-query="'.$encoded_query.'" data-drupal-link-system-path="query/test">' . check_plain('Link with query') . '</a></li>';

Note: We could use \Drupal::urlGenerate()->generate and String::checkPlain instead

Alumei’s picture

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

Alumei’s picture

Changed the json-test part to be route-based and use Drupal::urlGenerate()->generate() instead of url().

Daniel Norton’s picture

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

Daniel Norton’s picture

Assigned: Unassigned » Daniel Norton
Daniel Norton’s picture

Re-rerolled for PSR-4

Daniel Norton’s picture

Assigned: Daniel Norton » Unassigned

Status: Needs review » Needs work

The last submitted patch, 14: non-existent-class-json-2217755-14.patch, failed testing.

Daniel Norton’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: non-existent-class-json-2217755-14.patch, failed testing.

The last submitted patch, 14: non-existent-class-json-2217755-14.patch, failed testing.

Daniel Norton’s picture

Tests pass locally. Testbot is not providing sufficient detail to identify the failure.

martin107’s picture

Test 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()

martin107’s picture

Status: Needs work » Needs review
alexpott’s picture

Priority: Major » Critical

Given that this a php bug.

martin107’s picture

Status: Needs review » Needs work
Issue tags: +Needs a reroll

Patch no longer applies.

er.pushpinderrana’s picture

Status: Needs work » Needs review
Issue tags: -Needs a reroll
FileSize
3.36 KB

Rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 25: drupal8-non-existent-class-json-2217755-25.patch, failed testing.

star-szr’s picture

The rerolled patch shrank by quite a bit, before reroll was 6.75 KB and now 3.36 KB. Can you try again @er.pushpinderrana?

er.pushpinderrana’s picture

@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!

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
4.18 KB

One more try with updated patch.

er.pushpinderrana’s picture

@Cottser, this time Test Bot make it green :). Nice.

Thanks!

alimac’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch does not apply to HEAD, tagging Needs reroll.

error: patch failed: core/includes/theme.inc:8
error: core/includes/theme.inc: patch does not apply
alimac’s picture

Issue summary: View changes

Here are the reroll instructions.

Les Lim’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.2 KB

Patch didn't apply because it tried to put a 'use' statement in a place where another 'use' statement has since been added. Rerolled.

Les Lim’s picture

Issue tags: +TCDrupal 2014

Adding sprint tag.

Les Lim’s picture

New patch to keep 'use' statements grouped nicely.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 93beccc and pushed to 8.0.x. Thanks!

  • alexpott committed 93beccc on 8.0.x
    Issue #2217755 by Alumei, Les Lim, er.pushpinderrana, martin107, Daniel...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.