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

h3rj4n’s picture

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 ;)

tim.plunkett’s picture

h3rj4n’s picture

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.

tim.plunkett’s picture

Title:Convert filter_admin_format_page() to a Controller» Convert 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.

h3rj4n’s picture

Status:Needs work» Needs review

Lets test it.

h3rj4n’s picture

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.

tim.plunkett’s picture

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

Filter plugins went in.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

rerolled

tim.plunkett’s picture

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.

tim.plunkett’s picture

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!

dawehner’s picture

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

tim.plunkett’s picture

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.

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

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.

jibran’s picture

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.

dawehner’s picture

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

tim.plunkett’s picture

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.

dawehner’s picture

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.

tim.plunkett’s picture

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.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

tim.plunkett’s picture

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

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')

dawehner’s picture

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.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Looks like great updates based on what @alexpott requested.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed af5478b and pushed to 8.x. Thanks!

alexpott’s picture

Status:Fixed» Needs work

This broke head :(

alexpott’s picture

Reverted a9b5c81 and pushed to 8.x.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

kfritsche’s picture

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.

Gábor Hojtsy’s picture

Status:Needs review» Needs work
tim.plunkett’s picture

Status:Needs work» Needs review

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

kfritsche’s picture

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.

Gábor Hojtsy’s picture

The changes look good on visual review! Great refactoring.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

Gábor Hojtsy’s picture

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

YesCT’s picture

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.