Support from Acquia helps fund testing for Drupal Acquia logo

Comments

h3rj4n’s picture

And the patch

Status: Needs review » Needs work

The last submitted patch, drupal-filter_tips_long-1985528-1.patch, failed testing.

h3rj4n’s picture

Right way of adding new files with a patch.

h3rj4n’s picture

That one only contains the new files. This one should be the definite one.

h3rj4n’s picture

Status: Needs work » Needs review

forgot to set it to 'needs review'.

Status: Needs review » Needs work

The last submitted patch, drupal-filter_tips_long-1985528-4.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/filter/filter.routing.ymlundefined
@@ -4,3 +4,17 @@ filter_admin_disable:
+filter_tips:
+  pattern: '/filter/tips'
+  defaults:
+    _content: '\Drupal\filter\Controller\FilterController::filterTips'
+  requirements:
+    _access: 'TRUE'

If you want to reuse that method, you'll need filter_format: ~ under defaults, at the same level as _content.

Because your filterTips() method expects something to be passed, or it won't get called.

+++ b/core/modules/filter/lib/Drupal/filter/Access/FilterAccess.phpundefined
@@ -0,0 +1,44 @@
+    return FALSE;

Leave this off, it should return null.

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,55 @@
+  function filterTips($filter_format = NULL) {

The = NULL is not needed. Also this should be typehinted FilterFormat

h3rj4n’s picture

I changed everything you pointed out. The only thing I wasn't able to change was the typehinted part.

You mean that I should change the following:

+  function filterTips($filter_format) {

Into

+  function filterTips($FilterFormat) {

or am I mistaken?

If I change this the function doens't get a value passed along. I think it's because the name of the var isn't the same as the name used in the routing. But if I change the name in the routing I'm not sure if the proper load function is called on the input value.

h3rj4n’s picture

Status: Needs work » Needs review

Forgot to change the status.

Status: Needs review » Needs work

The last submitted patch, drupal-filter_tips_long-1985528-8.patch, failed testing.

h3rj4n’s picture

The patch still contains a white pace error ^_^

The patch wont pass the test because a non existing format should return a 404 instead of the current 200.

    // lines 159 and 160 of FilterFormatAccessTest.php
    $this->drupalGet('filter/tips/invalid-format');
    $this->assertResponse(404);

...

    // lines 170 and 171
    $this->drupalGet('filter/tips/invalid-format');
    $this->assertResponse(404);

I think the problem lays inside the Entity load because the access callback is never triggered. I cant find a way to custom send the 404 headers (as a test). I'm guessing that the Entity load fails and stops the code from executing and returning an empty page with headers set to 200 instead of 404.

I couldn't find an example where this already is fixed. I've find the following inside routings.yml files:

  • /admin/config/system/actions/configure/{number} - enter a wrong number and it fails, php error, so no example
  • /admin/structure/menu/item/{number}/reset - enter a wrong number, get a 200 back instead of a 404

If an entity can't be found Drupal returns a 200 instead of a 404. I changed this in the SimpleTest in the supplied patch.

h3rj4n’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-filter_tips_long_changed_test-1985528-11.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Postponed
tim.plunkett’s picture

Status: Postponed » Needs review
Issue tags: -WSCCI-conversion
h3rj4n’s picture

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-filter_tips_long_changed_test-1985528-11.patch, failed testing.

h3rj4n’s picture

Status: Needs work » Needs review

The patch from #8 is the right one.

tim.plunkett’s picture

+++ b/core/modules/filter/filter.routing.ymlundefined
@@ -4,3 +4,18 @@ filter_admin_disable:
+filter_tips:
+  pattern: '/filter/tips'
+  defaults:
+    _content: '\Drupal\filter\Controller\FilterController::filterTips'
...
+filter_tips_all:
+  pattern: '/filter/tips/{filter_format}'
+  defaults:
+    _content: '\Drupal\filter\Controller\FilterController::filterTips'

I still wish we'd be able to combine these two into one route. Now that those other bugs are fixed, maybe we should try again.

+++ b/core/modules/filter/filter.routing.ymlundefined
@@ -4,3 +4,18 @@ filter_admin_disable:
+    _access: 'TRUE'
+  ¶
+filter_tips_all:

Trailing whitespace

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,54 @@
+   * Page callback: Displays a page with long filter tips.

We're not using the Page callback: prefix anymore.

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,54 @@
+   * @param $format
+   *   (optional) A filter format. Defaults to NULL.
...
+  function filterTips($filter_format) {

This should be typehinted with \Drupal\filter\FilterFormatInterface

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,54 @@
+   * @see filter_menu()

Remove this

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,54 @@
+    return $build;
+  }

Missing a blank line before the end of the class.

ParisLiakos’s picture

Status: Needs review » Needs work

also:

+++ b/core/modules/filter/lib/Drupal/filter/Access/FilterAccess.phpundefined
@@ -0,0 +1,43 @@
+   * Implements \Drupal\Core\Access\AccessCheckInterface::applies().
...
+   * Implements \Drupal\Core\Access\AccessCheckInterface::access().

should be {@inheritdoc} now

h3rj4n’s picture

All the documentation I found on an optional variable pointed to a two lines inside the config file. Some of the resources:

http://stackoverflow.com/questions/10091518/symfony2-urls-with-trailing-slash-and-an-optional-parameter
http://stackoverflow.com/questions/11363103/multiple-pattern-in-single-symfony-routing

But after reading some more on the Symphony site I found the following:

But hold on! Since placeholders are required by default, this route will no longer match on simply /blog. Instead, to see page 1 of the blog, you'd need to use the URL /blog/1! Since that's no way for a rich web app to behave, modify the route to make the {page} parameter optional. This is done by including it in the defaults collection:

Source: http://symfony.com/doc/2.0/book/routing.html

So according to the documentation, this should work for both url's:

filter_tips_all:
  pattern: '/filter/tips/{filter_format}'
  defaults:
    _content: '\Drupal\filter\Controller\FilterController::filterTips'
    filter_format: 1
  requirements:
    _filter_access: 'TRUE'

But it doesn't. I tried the above code but it doesn't work as symphony says it should work. So I left it to two lines in the routing file.

h3rj4n’s picture

Status: Needs work » Needs review

Forgot to set it to needs review.

dawehner’s picture

+++ b/core/modules/filter/filter.routing.ymlundefined
@@ -4,3 +4,18 @@ filter_admin_disable:
\ No newline at end of file

There should be always a new line at the end of the files.

+++ b/core/modules/filter/lib/Drupal/filter/Access/FilterAccess.phpundefined
@@ -0,0 +1,43 @@
+  }

There should be an empty line before the last "}"

+++ b/core/modules/filter/lib/Drupal/filter/Controller/FilterController.phpundefined
@@ -0,0 +1,54 @@
+class FilterController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a FilterController object.
+   */
+  public function __construct() {

We don't inject anything, so let's skip all this code for now.

tim.plunkett’s picture

Cleaned up a bit. The route names were backwards.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/filter/lib/Drupal/filter/Access/FilterAccess.phpundefined
@@ -0,0 +1,43 @@
+      $permission = filter_permission_name($format);

This function seems to be a good candidate on the format object.

tim.plunkett’s picture

Well, we should decide if formats should themselves be checking filter_fallback_format(). If so, we have a lot we could move around.

quicksketch’s picture

Thanks for your work on this guys. Since we'll want to use the routing system for any new paths that need access checks against an individual filter format, this issue has become a dependency for #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, which needs to grant access to dialogs based on the text format.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll now that filters are plugins...

curl https://drupal.org/files/filter-tips-1985528-24.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5529  100  5529    0     0  42612      0 --:--:-- --:--:-- --:--:-- 49366
error: patch failed: core/modules/filter/filter.services.yml:10
error: core/modules/filter/filter.services.yml: patch does not apply
dawehner’s picture

Status: Needs work » Needs review
FileSize
5.54 KB

Rerole.

Status: Needs review » Needs work

The last submitted patch, drupal-1985528-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
555 bytes
5.54 KB

Urgs.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good

alexpott’s picture

Committed b0f09bb and pushed to 8.x. Thanks!

quicksketch’s picture

Status: Reviewed & tested by the community » Fixed

Yay, that makes this fixed. :)

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