Problem/Motivation

When no pages are specified, the summary prints out:

Return true on the following pages:

or

Do not return true on the following pages:

Whereas it should return an empty string signifying that it's not actually configured.

Steps to reproduce

Install the asset_injector module, and save a simple asset configuration without any conditions.
The summary will be inaccurate:
Inaccurate condition summary

Proposed resolution

The summary needs to be more accurate, returning an empty string if it's unconfigured.

Remaining tasks

Provide a patch/bugfix.

CommentFileSizeAuthor
#12 3202434-12.patch1.43 KBpaulocs
#12 interdiff-7-12.txt793 bytespaulocs
#7 3202434-7.patch1.48 KBpaulocs
#7 3202434-7-TEST-ONLY.patch794 bytespaulocs
#7 interdiff-2-7.txt1.38 KBpaulocs
#2 3202434-2.patch778 bytespaulocs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

codebymikey created an issue. See original summary.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Active » Needs review
FileSize
3202434-2.patch778 bytes
adalbertov’s picture

adalbertov CreditAttribution: adalbertov at CI&T commented
Status: Needs review » Reviewed & tested by the community

Hello folks, just checked this issue. After the patch the summary return looks better. Since the code is fine for me, I'm moving the issue to RTBC

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

Nitpick: you can check for empty before the implode().

Second nitpick: I think that configuration is usually talked about in the present tense, rather than past tense? I don't see anything about that in https://www.drupal.org/docs/develop/user-interface-standards/interface-text though.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This could also use some test coverage.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented

Ok, I'll provide a new patch.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Needs review
FileSize
interdiff-2-7.txt1.38 KB
3202434-7-TEST-ONLY.patch794 bytes
3202434-7.patch1.48 KB

The last submitted patch, 7: 3202434-7-TEST-ONLY.patch, failed testing. View results

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Status: Needs review » Reviewed & tested by the community

Test-only patch returns specific error, and patch works as intended.

Moving to RTBC.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Issue tags: -Needs tests
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Plugin/Condition/RequestPathTest.php
@@ -88,6 +88,7 @@ public function testConditions() {
     $condition = $this->pluginManager->createInstance('request_path');
+    $this->assertEquals('No page is specified', $condition->summary(), 'The condition summary matches when no path is set');
     $condition->setConfig('pages', $pages);

I don't think we need the custom assertion message here - can just use phpunit's default one.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Needs review
FileSize
interdiff-7-12.txt793 bytes
3202434-12.patch1.43 KB

I removed the custom message parameter in the $this->assertEquals function.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 030bfb8 on 9.2.x 
    Issue #3202434 by paulocs, codebymikey, catch, guilhermevp, joachim: The...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

  • catch committed 79425b4 on 9.1.x 
    Issue #3202434 by paulocs, codebymikey, catch, guilhermevp, joachim: The...

Status: Fixed » Closed (fixed)

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