Support from Acquia helps fund testing for Drupal Acquia logo

Comments

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
30.74 KB

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

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +MENU_LOCAL_ACTION
h3rj4n’s picture

Status: Needs work » Needs review
FileSize
30.7 KB

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
FileSize
25.9 KB
53.18 KB

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

+++ 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
FileSize
35.91 KB
55.3 KB

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

tim.plunkett’s picture

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

FileSize
56.17 KB

rerolled

tim.plunkett’s picture

FileSize
1.59 KB
56.66 KB

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

FileSize
817 bytes
57.46 KB

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
FileSize
6.16 KB
59.41 KB

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
FileSize
4.21 KB
60.66 KB

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

FileSize
27.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

FileSize
3.8 KB
62.9 KB

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
FileSize
511 bytes
62.91 KB

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
FileSize
2.96 KB
63.12 KB

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
FileSize
2.07 KB
64.01 KB

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

FileSize
804 bytes
64.21 KB

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

FileSize
9.58 KB
66.2 KB

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.