Files: 
CommentFileSizeAuthor
#50 filter-1987124-50.patch66.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,031 pass(es).
[ View ]
#50 interdiff.txt9.58 KBtim.plunkett
#46 filter-1987124-46.patch64.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,262 pass(es).
[ View ]
#46 interdiff.txt804 bytestim.plunkett
#44 filter-1987124-44.patch64.01 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,867 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#44 interdiff.txt2.07 KBtim.plunkett
#39 drupal-1987124-39.patch63.12 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,253 pass(es).
[ View ]
#39 interdiff.txt2.96 KBdawehner
#32 filter-1987124-32.patch62.91 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,008 pass(es).
[ View ]
#32 interdiff.txt511 bytestim.plunkett
#29 filter-1987124-29.patch62.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#29 interdiff.txt3.8 KBtim.plunkett
#28 edit.png27.99 KBdawehner
#25 filter-1987124-25.patch60.66 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 49 fail(s), and 10 exception(s).
[ View ]
#25 interdiff.txt4.21 KBtim.plunkett
#22 filter-1987124-22.patch59.41 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,822 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#22 interdiff.txt6.16 KBtim.plunkett
#20 filter-1987124-20.patch57.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,864 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#20 interdiff.txt817 bytestim.plunkett
#18 filter-1987124-18.patch56.66 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,720 pass(es), 9 fail(s), and 7 exception(s).
[ View ]
#18 interdiff.txt1.59 KBtim.plunkett
#16 filter-1987124-16.patch56.17 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,334 pass(es), 26 fail(s), and 5 exception(s).
[ View ]
#14 filter-admin-1987124-14.patch56.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,018 pass(es), 10 fail(s), and 5 exception(s).
[ View ]
#14 interdiff.txt2.02 KBtim.plunkett
#12 filter-1987124-12.patch55.3 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,865 pass(es), 34 fail(s), and 15 exception(s).
[ View ]
#12 interdiff.txt35.91 KBtim.plunkett
#9 drupal-filter_admin_format_page-1987124-9.patch53.17 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 55,517 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#6 filter-1987124-6.patch53.18 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,347 pass(es), 199 fail(s), and 85 exception(s).
[ View ]
#6 interdiff.txt25.9 KBtim.plunkett
#4 drupal-filter_admin_format_page-1987124-4.patch30.7 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 55,569 pass(es), 19 fail(s), and 1 exception(s).
[ View ]
#1 drupal-filter_admin_format_page-1987124-1.patch30.74 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 55,476 pass(es), 135 fail(s), and 26 exception(s).
[ View ]

Comments

Status:Needs work» Needs review
StatusFileSize
new30.74 KB
FAILED: [[SimpleTest]]: [MySQL] 55,476 pass(es), 135 fail(s), and 26 exception(s).
[ View ]

And the patch. See if this one doesn't fail ;)

Status:Needs work» Needs review
StatusFileSize
new30.7 KB
FAILED: [[SimpleTest]]: [MySQL] 55,569 pass(es), 19 fail(s), and 1 exception(s).
[ View ]

After reading this page I got it working. Don't think this would be approved to be the actual patch but it's working. I just hope it passes all the tests.

I added the following in the route.yml:

filter_format_add:
  pattern: '/admin/config/content/formats/{notexisting}'
  defaults:
    _form: '\Drupal\filter\Form\FilterAdmin'
    notexisting: ~
  requirements:
    notexisting: '[ad]{3}'
    _permission: 'administer filters'
filter_format_edit:
  pattern: '/admin/config/content/formats/{filter_format}'
  defaults:
    _form: '\Drupal\filter\Form\FilterAdmin'
  requirements:
    _permission: 'administer filters'

This way the url formats/all and formats/basic_html both works. Replacing {notexisting} with add will break this.

Title:Convert filter_admin_format_page() to a ControllerConvert filter_admin_format_page() and filter_admin_overview() to a Controller
Status:Needs review» Needs work
StatusFileSize
new25.9 KB
new53.18 KB
FAILED: [[SimpleTest]]: [MySQL] 56,347 pass(es), 199 fail(s), and 85 exception(s).
[ View ]

So we just need to fix the paths.

However, these should be EntityFormController subclasses and use _entity_form.

Status:Needs work» Needs review

Lets test it.

StatusFileSize
new53.17 KB
FAILED: [[SimpleTest]]: [MySQL] 55,517 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

+++ b/core/modules/filter/filter.routing.yml
@@ -1,6 +1,27 @@
+filter_format_add:
+  pattern: '/admin/config/content/formats/manage/add'
+  defaults:

The routing.yml contained the wrong route. It should be /admin/config/content/formats/add. Otherwise it isn't consistent.

Added the patch.

Component:routing system» filter.module
Assigned:Unassigned» tim.plunkett
Status:Needs review» Needs work

Filter plugins went in.

Status:Needs work» Needs review
StatusFileSize
new35.91 KB
new55.3 KB
FAILED: [[SimpleTest]]: [MySQL] 55,865 pass(es), 34 fail(s), and 15 exception(s).
[ View ]

Too much changed for a proper interdiff, I just diffed the patches.

StatusFileSize
new2.02 KB
new56.22 KB
FAILED: [[SimpleTest]]: [MySQL] 56,018 pass(es), 10 fail(s), and 5 exception(s).
[ View ]

Well, I think the remaining bug is a problem.

I was noticing that $form_state['controller']->getEntity() was not actually the one that had just been saved.

I had debug(spl_object_hash($this)) in the add form, and debug($form_state['controller']); and they were different.

StatusFileSize
new56.17 KB
FAILED: [[SimpleTest]]: [MySQL] 56,334 pass(es), 26 fail(s), and 5 exception(s).
[ View ]

rerolled

StatusFileSize
new1.59 KB
new56.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,720 pass(es), 9 fail(s), and 7 exception(s).
[ View ]

Sloppy reroll.
Still seeing the problem described in #14.

EDIT:
Steps to reproduce:

  • Install standard profile
  • Go to admin/config/content/formats/add
  • Give it a label
  • Choose "CKEditor" from the "Text editor" select list
  • Click save

editor_form_filter_format_form_alter() and editor_form_filter_admin_format_submit() are the problems.

StatusFileSize
new817 bytes
new57.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,864 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

dawehner is my hero. He found the bug!

Status:Needs review» Needs work

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,207 @@
+      '#weight' => -30,
...
+      '#weight' => -20,
...
+      '#weight' => -10,

I like this pattern of making it easy to place form elements between the existing ones.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,207 @@
+      '#options' => array_map('check_plain', user_role_names()),

another check plain.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,207 @@
+    elseif ($admin_role = config('user.settings')->get('admin_role')) {

Another injection.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,207 @@
+    // @todo Move trimming upstream.

I guess it just makes sense for all machine names?

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,137 @@
+  public function load() {
...
+    return array_filter(parent::load(), function ($entity) {
+      return $entity->status();
+    });

So you will never be able to enable an input format via the UI because it is not listed? I see that filter_formats() does the same, so that is not a problem.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,137 @@
+        if (config('filter.settings')->get('always_show_fallback_choice')) {

Just a nitpick: config could be injected.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,137 @@
+        $form['formats'][$id]['name'] = array('#markup' => check_plain($format->name));
+        $roles = array_map('check_plain', filter_get_roles_by_format($format));
+        $roles_markup = $roles ? implode(', ', $roles) : t('No roles may use this format');

check_plain now should be replaced by String::checkPlain

Status:Needs work» Needs review
StatusFileSize
new6.16 KB
new59.41 KB
FAILED: [[SimpleTest]]: [MySQL] 56,822 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Replaced/injected all the things.

yes, the weights is a nice touch, that's already in HEAD.

And you cannot delete a text format, and once you disable it, you can't re-enable it. Kinda strange, but that's how it works.

Status:Needs review» Needs work
Issue tags:+D8MI, +language-config

Woot! I just got here as #2023747: Use EntityListController for formats was marked a duplicate. D8MI needs this conversion so we can alter the operations list in a standard way and provide input format translation (names of formats are part of the content submission frontend).

Status:Needs work» Needs review
StatusFileSize
new4.21 KB
new60.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 49 fail(s), and 10 exception(s).
[ View ]

Great, a full review would be nice now that this patch should be green.

Some minors

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,251 @@
+   * Constructs a new FilterFormatFormControllerBase.

@param missing.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,172 @@
+   * {@inheritdoc}

doc required.

StatusFileSize
new27.99 KB

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,172 @@
+        'title' => t('configure'),
...
+          'title' => t('disable'),

Aren't we going with uppercase now?

One problem I had when manually trying out the patch is the following:
edit.png

StatusFileSize
new3.8 KB
new62.9 KB
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I broke the validate handler, but the fix needed changes to config entity_query. I updated the tests to include coverage for my change.
I also address the docs issues, thanks @jibran.

Status:Needs review» Reviewed & tested by the community

The addition to config EQ is perfect. The validation now works as expected, so this is ready to fly.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new511 bytes
new62.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,008 pass(es).
[ View ]

Silly mistake. Yay test coverage.

Status:Needs review» Reviewed & tested by the community

Wow I wouldn't have expected that filter_format_load is not just a simple wrapper.

Status:Reviewed & tested by the community» Needs review

#32: filter-1987124-32.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -391,6 +391,8 @@ protected function submitEntityLanguage(array $form, array &$form_state) {
+    // The form state might still have an form controller before form caching.
+    $form_state['controller'] = $this;

This could do with a bit more explanation since "might" is a pretty strange word to use. We need to comment on the condition that makes this required... @timplunkett thinks it might be because of during a form alter inside an ajax callback but he was not sure.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatFormControllerBase.phpundefined
@@ -0,0 +1,257 @@
+    $is_fallback = ($format->id() == filter_fallback_format());
+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.phpundefined
@@ -0,0 +1,183 @@
+    $fallback_format = filter_fallback_format();

This can use the injected config... all this filter_fallback_format() does is return config('filter.settings')->get('fallback_format')

Status:Needs work» Needs review
StatusFileSize
new2.96 KB
new63.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,253 pass(es).
[ View ]

Here is a longer explanation.

Status:Needs review» Reviewed & tested by the community

Looks like great updates based on what @alexpott requested.

Status:Reviewed & tested by the community» Fixed

Committed af5478b and pushed to 8.x. Thanks!

Status:Fixed» Needs work

This broke head :(

Reverted a9b5c81 and pushed to 8.x.

Status:Needs work» Needs review
StatusFileSize
new2.07 KB
new64.01 KB
FAILED: [[SimpleTest]]: [MySQL] 56,867 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

This needed a reroll after #2027183: hook_menu() title callback is ignored on routes, it was coding around that bug and when it was fixed, things broke.
Also switched to hook_local_actions() now that we can.

Sorry for the broken HEAD.

StatusFileSize
new804 bytes
new64.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,262 pass(es).
[ View ]

Not sure why this ever passed, but this was the only direct call to drupal_static_reset('filter_formats'), and replacing it with the proper filter_formats_reset() fixes the test.

Sorry to just hope in here, but I tried to already do follow ups as this was shortly in.
But so I reviewed this patch indirectly.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatListController.php
@@ -0,0 +1,184 @@
+  public function buildForm(array $form, array &$form_state) {
...
+    foreach ($this->entities as $id => $format) {

The following code should be moved to the buildRow() method. This method should call parent:buildRow(), like we use it in many other ListControllers. Without parent:buildRow() hook_entity_operation_alter() is never called.

edit:
Also $this->buildOperations($entity) should be used in there to create the operations link array.

Status:Needs review» Needs work

Status:Needs work» Needs review

That's a good idea, and the code will become much more readable. Doing so now.

StatusFileSize
new9.58 KB
new66.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,031 pass(es).
[ View ]

Okay, that's done now. This is almost identical to RoleListController, which is a good sign.

Note: buildRow() is not needed at all for hook_entity_operation_alter() to be called, only buildOperations().
That's why it's not a problem that I don't call parent::buildRow(), as long as EntityListController::buildOperations() runs.

Thanks for hinting it out and you are completely right, but buildOperations() is needed for that. And with buildRow() its more readable.
If this goes green now we should be fine.
Good work.

The changes look good on visual review! Great refactoring.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed 93ea9a4 and pushed to 8.x. Thanks!

Yayay! This makes config translation work for filter formats easily now. We can remove lots of ugly code from there. Yay!

a couple of days ago, while manually testing this, I noticed: #2029219: Change the disable user interface with formats. Make it remove.
it could use some help finding the appropriate related issues, and some good tags. Perhaps it is a duplicate, or wont fix.

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