To make urls more readable, we should be able to change an alias for a facet.

CommentFileSizeAuthor
#10 create_settings_for_the-2624410-10.patch9.45 KBborisson_
#9 create_settings_for_the-2624410-9.patch7.16 KBborisson_
#8 create_settings_for_the-2624410-8.patch6.96 KBborisson_
#7 create_settings_for_the-2624410-7.patch6.88 KBborisson_
#7 interdiff.txt6.34 KBborisson_
#5 create_settings_for_the-2624410-5.patch8.13 KBborisson_
#5 interdiff.txt1.15 KBborisson_
#2 create_settings_for_the-2624410-2.patch8.07 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Active » Needs review
FileSize
create_settings_for_the-2624410-2.patch8.07 KB

How bout this?

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/src/Form/FacetForm.php
@@ -174,6 +174,15 @@ class FacetForm extends EntityForm {
+      '#description' => $this->t('Enter the name of the facet for usage in URLs, this has to adhere to the URL standards.'),

    Which URL standards? This help text does not add much without more context. If we will url encode that anyway, what does it even matter? 

  2. +++ b/src/Plugin/facetapi/processor/QueryStringUrlProcessor.php
@@ -58,9 +66,11 @@ class QueryStringUrlProcessor extends UrlProcessorPluginBase {
+    $this->url_parameter = $facet->getUrlParameter();

    Don't we need to xss filter the url parameter as well? 

  3. +++ b/tests/src/Unit/Plugin/processor/QueryStringUrlProcessorTest.php
@@ -107,9 +107,29 @@ class QueryStringUrlProcessorTest extends UnitTestCase {
+    $request->query->set('f', []);

    an array is required? 

  4. +++ b/tests/src/Unit/Plugin/processor/QueryStringUrlProcessorTest.php
@@ -107,9 +107,29 @@ class QueryStringUrlProcessorTest extends UnitTestCase {
+      $this->assertEquals('route:test?f[0]=aah%3A' . $r->getRawValue(), $r->getUrl()->toUriString());

    url encoding :( Always ugly, but understandable.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

#3.1 - Removing the description looks like the best thing to do.
#3.2 - Yes, we should. I'll have a look at how the validation of the path widget works.
#3.3 - This is only in the test, but yeah; the \Symfony\Component\HttpFoundation\ParameterBag::set method needs a key and a value.
#3.4 - Nothing to do about that.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.15 KB
create_settings_for_the-2624410-5.patch8.13 KB

I did the Xss in the entity, for better results.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Status: Needs review » Needs work

I dont think we need to do an xss filter on this field (and if we do, we shouldnt do it in the set-method, Drupal always filters when requesting data, not when setting data)
I might be wrong though, can somebody give an example on how this can be abused?

url_parameter: can we use url_alias instead? D7 Pretty paths used this too, and I think its a better name ;)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt6.34 KB
create_settings_for_the-2624410-7.patch6.88 KB

Changed to url_alias and removed the xss filtering.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
create_settings_for_the-2624410-8.patch6.96 KB

Needed a reroll because of namespace changes.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
create_settings_for_the-2624410-9.patch7.16 KB

Needed another reroll.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
create_settings_for_the-2624410-10.patch9.45 KB

Added a test, tests are good.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

If you open a dev release you can test your patches, makes it easier...

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

@aspilicious: true, but the tests still require search api, while the module doesn't, per se. In the travis tests, we explicitly enable search_api before executing tests.

- http://cgit.drupalcode.org/facets/tree/.travis.yml

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

There is a workaround for that, I do the same for field_group in DS. Create a fake test module with a dependency on search_API nothibg else. Commit and wait 24 hours. Then testbot will not fail anymore. :)

Look at the commit log for DS if you need an example.

  • borisson_ committed 1ffa3d3 on 8.x-1.x 
    Issue #2624410 by borisson_: Create settings for the url processor to...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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