Hi,

On a project I'm working on we needed a plugin condition for the domain name and a colleague suggested it might be suitable for ctools.

Thanks

CommentFileSizeAuthor
#11 interdiff_10-11.txt588 bytesAndyF
#11 ctools-domain_name_condition_plugin-2898191-11-D8.patch7.33 KBAndyF
#10 ctools-Add_a_condition_plugin_for_domain_name-2898191-10-D8.patch7.32 KBthalles
#10 interdiff_5-10.txt1.81 KBthalles
#8 interdiff_5-8.txt1.54 KBthalles
#8 ctools-domain_name_condition_plugin-2898191-8-D8.patch7.27 KBthalles
#7 interdiff_5-7.txt6.48 KBthalles
#7 ctools-domain_name_condition_plugin-2898191-7-D8.patch556 bytesthalles
#5 ctools-domain_name_condition_plugin-2898191-5-D8.patch7.24 KBAndyF
#2 ctools-domain_name_condition_plugin-2898191-2-D8.patch6.14 KBAndyF
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AndyF created an issue. See original summary.

AndyF’s picture

AndyF CreditAttribution: AndyF at TES Global commented
Status: Active » Needs review
FileSize
ctools-domain_name_condition_plugin-2898191-2-D8.patch6.14 KB

Thanks

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and for TES Global commented 
  1. +++ b/src/Plugin/Condition/Domain.php
@@ -0,0 +1,118 @@
+    $this->configuration['domains'] = implode("\n", $domains);

    Is there a reason we couldn't store a list of domains instead of store them as a string? 

  2. +++ b/tests/src/Kernel/DomainTest.php
@@ -0,0 +1,82 @@
+class DomainTest extends KernelTestBase {

    Kudos for writing a test!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and for TES Global commented

It also sounds like we are missing some config dependency change missing.

AndyF’s picture

AndyF CreditAttribution: AndyF at TES Global commented
FileSize
ctools-domain_name_condition_plugin-2898191-5-D8.patch7.24 KB

@dawehner thanks for the review!

Is there a reason we couldn't store a list of domains instead of store them as a string?

Not really, it's a hangover from basing it on \Drupal\system\Plugin\Condition\RequestPath. I've updated it.

It also sounds like we are missing some config dependency change missing.

Done, thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and for TES Global commented
Status: Needs review » Reviewed & tested by the community

This looks great for me!

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
Status: Reviewed & tested by the community » Needs review
FileSize
ctools-domain_name_condition_plugin-2898191-7-D8.patch556 bytes
interdiff_5-7.txt6.48 KB

Follow the patch!

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
FileSize
ctools-domain_name_condition_plugin-2898191-8-D8.patch7.27 KB
interdiff_5-8.txt1.54 KB

Follow the new patch!

Status: Needs review » Needs work

The last submitted patch, 8: ctools-domain_name_condition_plugin-2898191-8-D8.patch, failed testing. View results

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented
Status: Needs work » Needs review
FileSize
interdiff_5-10.txt1.81 KB
ctools-Add_a_condition_plugin_for_domain_name-2898191-10-D8.patch7.32 KB

Follow the patch!

AndyF’s picture

AndyF CreditAttribution: AndyF at Fabb commented
FileSize
ctools-domain_name_condition_plugin-2898191-11-D8.patch7.33 KB
interdiff_10-11.txt588 bytes

Thanks! And one last bit of follow-up cleaning: if splitting an array over multiple lines, each element should be on a separate one. If the tests run green I think this can be moved back to RTBC as the changes since #5 are small.

@thalles For what it's worth, I find the phrase "follow the patch" a little meaningless - from my PoV it would be great to describe what you're doing and why when updating an issue - but thanks for your time and attention!

AndyF’s picture

AndyF CreditAttribution: AndyF at Fabb commented
Status: Needs review » Reviewed & tested by the community

Moving to RTBC per #5, see interdiff_5-10.txt and interdiff_10-11.txt which just include a typehint for the request stack and a couple of code formatting touch-ups.

Thanks!

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
CreditAttribution: thalles at CI&T commented

Thanks @AndyF!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Reviewed & tested by the community » Closed (works as designed)

I don't think I'd like to add this to ctools as it seems a narrow case but I encourage you to add a ctools_domain module.