Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Status: Active » Needs review
FileSize
8.07 KB

How bout this?

Nick_vh’s picture

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

#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

Status: Needs work » Needs review
FileSize
1.15 KB
8.13 KB

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

StryKaizer’s picture

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

Status: Needs work » Needs review
FileSize
6.34 KB
6.88 KB

Changed to url_alias and removed the xss filtering.

borisson_’s picture

FileSize
6.96 KB

Needed a reroll because of namespace changes.

borisson_’s picture

Needed another reroll.

borisson_’s picture

Added a test, tests are good.

aspilicious’s picture

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

borisson_’s picture

@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

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

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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