Files: 
CommentFileSizeAuthor
#31 drupal-1985528-31.patch5.54 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,764 pass(es).
[ View ]
#31 interdiff.txt555 bytesdawehner
#29 drupal-1985528-29.patch5.54 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 filter-tips-1985528-24.patch5.4 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]
#24 interdiff.txt3.11 KBtim.plunkett
#21 drupal-filter_tips_long-1985528-21.patch5.69 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 55,624 pass(es).
[ View ]
#11 drupal-filter_tips_long_changed_test-1985528-11.patch6.82 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 55,824 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#8 drupal-filter_tips_long-1985528-8.patch5.82 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 55,850 pass(es).
[ View ]
#4 drupal-filter_tips_long-1985528-4.patch5.82 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 56,056 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#3 drupal-filter_tips_long-1985528-3.patch2.87 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 drupal-filter_tips_long-1985528-1.patch5.91 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-filter_tips_long-1985528-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new5.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-filter_tips_long-1985528-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And the patch

Status:Needs review» Needs work

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

StatusFileSize
new2.87 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Right way of adding new files with a patch.

StatusFileSize
new5.82 KB
FAILED: [[SimpleTest]]: [MySQL] 56,056 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

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

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.

+++ 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

StatusFileSize
new5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,850 pass(es).
[ View ]

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.

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.

StatusFileSize
new6.82 KB
FAILED: [[SimpleTest]]: [MySQL] 55,824 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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.

Assigned:Unassigned» tim.plunkett
Status:Needs work» Postponed

Status:Postponed» Needs review
Issue tags:-WSCCI-conversion

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

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

Status:Needs work» Needs review

The patch from #8 is the right one.

+++ 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.

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

StatusFileSize
new5.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,624 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Forgot to set it to needs review.

+++ 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.

StatusFileSize
new3.11 KB
new5.4 KB
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]

Cleaned up a bit. The route names were backwards.

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.

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new5.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerole.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new555 bytes
new5.54 KB
PASSED: [[SimpleTest]]: [MySQL] 56,764 pass(es).
[ View ]

Urgs.

Status:Needs review» Reviewed & tested by the community

looks good

Committed b0f09bb and pushed to 8.x. Thanks!

Status:Reviewed & tested by the community» Fixed

Yay, that makes this fixed. :)

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