Problem/Motivation

Previously we used filter_list_format() to load formats and filters.
Now that they are entities, we don't need an extra function for static caching.

Proposed resolution

Remove unnecessary code from the filter module.

Remaining tasks

None.

User interface changes

None.

API changes

Removed the following functions

  • filter_format_load() use entity_load instead
  • filter_format_exists() use use entity_load instead
  • filter_permission_name() use getPermissionName() method on the filter format entity instead
  • filter_list_format() use filters() method on the filter format entity instead (becomes: $format = entity_load('filter_format', $format_id)
  • filter_access() use access() method on the filter format entity instead
  • FilterAccessCheck class is removed replaced with entity access check

Changed the following function signatures

  • filter_formats() type hints $account parameter with AccountInterface
  • filter_get_roles_by_format() type hints $format with FilterFormatInterface
  • filter_default_format() type hints $account with AccountInterface
CommentFileSizeAuthor
#79 filter-2012312-79.patch55.17 KBtim.plunkett
#76 filter-2012312-76.patch54.29 KBtim.plunkett
#76 interdiff.txt932 bytestim.plunkett
#70 filter-2012312-70.patch54.29 KBtim.plunkett
#70 interdiff.txt2.21 KBtim.plunkett
#68 interdiff.txt759 bytestim.plunkett
#68 filter-2012312-68.patch56.01 KBtim.plunkett
#66 filter-2012312-66.patch55.27 KBtim.plunkett
#66 interdiff.txt1.53 KBtim.plunkett
#65 interdiff.txt931 bytestim.plunkett
#65 filter-2012312-65.patch55.68 KBtim.plunkett
#61 filter-2012312-61.patch55.42 KBtim.plunkett
#61 interdiff.txt2.79 KBtim.plunkett
#58 interdiff.txt2.88 KBtim.plunkett
#56 filter-2012312-56.patch55.8 KBtim.plunkett
#55 interdiff.txt1.34 KBtim.plunkett
#55 filter-2012312-55.patch55.92 KBtim.plunkett
#51 filter-2012312-51.patch54.96 KBtim.plunkett
#51 interdiff.txt1.54 KBtim.plunkett
#47 filter-2012312-47.patch54.67 KBtim.plunkett
#45 filter-2012312-45.patch54.67 KBtim.plunkett
#45 interdiff.txt751 bytestim.plunkett
#43 filter-2012312-43.patch54.63 KBtim.plunkett
#43 interdiff.txt1.64 KBtim.plunkett
#39 interdiff.txt2.03 KBtim.plunkett
#39 filter-2012312-39.patch54.48 KBtim.plunkett
#37 filter-2012312-37.patch54.13 KBtim.plunkett
#36 filter-2012312-36.patch53.5 KBtim.plunkett
#36 interdiff.txt741 bytestim.plunkett
#34 filter-2012312-34.patch53.42 KBtim.plunkett
#34 interdiff.txt4.87 KBtim.plunkett
#32 filter-2012312-32.patch52.81 KBtim.plunkett
#32 interdiff.txt734 bytestim.plunkett
#30 filter-2012312-30.patch52.1 KBtim.plunkett
#28 filter-2012312-28.patch53.76 KBtim.plunkett
#28 interdiff.txt2.56 KBtim.plunkett
#26 filter-2012312-26.patch54.39 KBtim.plunkett
#26 interdiff.txt11.8 KBtim.plunkett
#20 filter-2012312-20.patch52.1 KBtim.plunkett
#20 interdiff.txt2.03 KBtim.plunkett
#16 filter-2012312-16.patch51.3 KBtim.plunkett
#16 interdiff.txt5.28 KBtim.plunkett
#12 filter-2012312-11.patch47.46 KBtim.plunkett
#12 interdiff.txt1.63 KBtim.plunkett
#9 filter-2012312-9.patch47.41 KBtim.plunkett
#9 interdiff.txt2.34 KBtim.plunkett
#6 filter-2012312-6.patch46.98 KBtim.plunkett
#6 interdiff.txt905 bytestim.plunkett
#4 filter-2012312-4.patch46.57 KBtim.plunkett
#4 interdiff.txt580 bytestim.plunkett
#2 filter-2012312-2.patch46 KBtim.plunkett
#2 interdiff.txt39 KBtim.plunkett
#1 filter-2012312-1.patch9.02 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
9.02 KB

Let's see if this passes

tim.plunkett’s picture

Title: Remove filter_list_format($format_id) in favor of entity_load('filter_format', $format_id) » Remove functions that wrap entity_load('filter_format', $format_id)
FileSize
39 KB
46 KB

xjm suggested expanding the scope to include some of the other functions.

filter_permission_name($format) => $format->getPermissionName();
filter_list_format($format_id)  => entity_load('filter_format', $format_id)->filters()
filter_access($format)          => $format->access()

Also, I trimmed down filter_formats() a good deal, but didn't kill it outright yet. It filters out disabled filters and sorts them, and stores per user their access. Not sure where to put that.

tim.plunkett’s picture

FileSize
580 bytes
46.57 KB

Left in a stale service definition.

tim.plunkett’s picture

FileSize
905 bytes
46.98 KB

Workaround since global $user is not always (ever?) an instance of UserInterface.

tim.plunkett’s picture

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

Status: Needs work » Needs review
FileSize
2.34 KB
47.41 KB

Ah, the UserNG patch also changed the typehint in EntityAccessController.

11,779 exceptions! That's a respectable number.

Still a couple fails, but not sure where yet.

tim.plunkett’s picture

FileSize
1.63 KB
47.46 KB

Fixed two more bugs.

tim.plunkett’s picture

FileSize
5.28 KB
51.3 KB

Okay, finally fixed those.

Berdir’s picture

+++ b/core/modules/filter/filter.moduleundefined
@@ -155,23 +156,19 @@ function filter_menu() {
+ * @deprecated Only used as a menu loader. Use
+ *   entity_load('filter_format', $format_id) instead.

Hm.

I'm very +1 on dropping functions that can be replaced by methods (like the getPermissionName() that you are adding here) or are wrappers for methods like save().

That said, the advantage of something like filter_format_load() as a wrapper for entity_load() is that it allows to document the returned interface and gives you autocomplete on those methods.

When we can inject it into controllers/handlers/.. then we can type hint there, but that's not always possibly and filter formats sounds like a case that will have quite some usage in alter hooks, form process thingies and so on.

Also a bit worried about performance, we need to check that we don't introduce a regression here, not only with the default format but also on sites with 5-10 formats and various permissions. A lot of thought went into caching here and even with my config storage loadMultiple() issue, that's still slower than a direct cache on this level, as we always re-create the entity objects every time it's accessed, just as one example.

tim.plunkett’s picture

FileSize
2.03 KB
52.1 KB

It's only used in

filter_fallback_format_title()
filter_format_allowcache()
check_markup()
Drupal\editor\Plugin\Core\Entity\Editor::label()

filter_fallback_format_title() has been unused since #635212: Fallback format is not configurable in June 2010.
filter_format_allowcache() is used only by text module. Seems pretty lame.
Editor::label(), whatever.

So check_markup() is the one I think we care about.
How about this?

tim.plunkett’s picture

#20: filter-2012312-20.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Needs work
Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module'  [error]
cannot be null: INSERT INTO {role_permission} (rid, permission, module) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1,
:db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => authenticated
    [:db_insert_placeholder_1] => use text format basic_html
    [:db_insert_placeholder_2] => 
)
 in Drupal\Core\Database\Connection->query() (line 568 of /Users/timplunk/www/d8/core/lib/Drupal/Core/Database/Connection.php).

Um. I have no idea. I liked the patch in #16 better.

Berdir’s picture

That error usually happens if the permission name can't be looked up but I'm not sure why that would happen with those changes.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.8 KB
54.39 KB

This is a reroll of #16 for now.

tim.plunkett’s picture

Title: Remove functions that wrap entity_load('filter_format', $format_id) » Remove legacy code from filter.module
FileSize
2.56 KB
53.76 KB

Fixed most of that.

tim.plunkett’s picture

Parts of this will go in as part of #2030129: FilterFormat has no access controller

tim.plunkett’s picture

FileSize
52.1 KB

That went in.

tim.plunkett’s picture

FileSize
734 bytes
52.81 KB

Missed a spot.

tim.plunkett’s picture

FileSize
4.87 KB
53.42 KB

Rerolled.

tim.plunkett’s picture

FileSize
741 bytes
53.5 KB

Whoops, too aggressive

tim.plunkett’s picture

FileSize
54.13 KB

Rerolled.

jibran’s picture

Status: Needs review » Needs work
+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.phpundefined
index 1a4981a..f3d4e38 100644
--- a/core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php

This class needs some injection but I think it is out of scope. Maybe follow up.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.phpundefined
@@ -68,9 +68,7 @@ protected function textFormatHasTransformationFilters($format_id) {
+    $user_format_ids = array_keys(filter_formats(user_load($GLOBALS['user']->uid)));

Can we use Drupal::request()->attributes->get('account')->id() here?

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatAccessController.phpundefined
@@ -26,13 +26,13 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+    if ($operation != 'view' && user_access('administer filters', $account)) {
...
     return !empty($permission) && user_access($permission, $account);

$account->hasPermission() can be used here.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
54.48 KB
2.03 KB

Editor module has almost nothing injected, and that class alone has 3 calls to \Drupal already.
I fixed everything else.

jibran’s picture

It is all about code removal. So I think If it is green and with no coding issue it is RTBC. Let's wait for @Berdir for RTBC.

dawehner’s picture

+++ b/core/modules/filter/filter.moduleundefined
@@ -206,11 +169,11 @@ function filter_permission() {
+      $format_name_replacement = user_access('administer filters') ? l($format->label(), 'admin/config/content/formats/manage/' . $format->id()) : drupal_placeholder($format->label());

I seriously still scares me to use user_access in hook_permission

I guess we should use $account->hasPermission and String::placeholder now.

+++ b/core/modules/filter/filter.moduleundefined
@@ -594,69 +530,23 @@ function filter_fallback_format() {
 function filter_format_allowcache($format_id) {
-  $format = filter_format_load($format_id);
+  $format = $format_id ? entity_load('filter_format', $format_id) : FALSE;

Just wondering why anything calls the method with an invalid parameter...

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAdminTest.phpundefined
index 15ae75c..77965c4 100644
--- a/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.php

--- a/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.php
+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.phpundefined

I am a bit confused about the removal of test coverage given that this indeed tests something useful: are the text filters associated with a text format stored correctly.

Status: Needs review » Needs work

The last submitted patch, filter-2012312-39.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
54.63 KB

As part of this conversion, we're replacing filter_list_formats($format->format) with $format->filters()

If I update the one test:

function verifyFilters($format) {
-    $filters = filter_list_format($format->format);
+    $filters = $format->filters();
     $format_filters = $format->filters();

We're then checking $format->filters() against itself... So I'm okay with removing that.

The other test I removed is comparing the results of entity_create() to entity_load(), which is a little more valid. Restored.

Status: Needs review » Needs work

The last submitted patch, filter-2012312-43.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
751 bytes
54.67 KB

This is getting ridiculous

dawehner’s picture

-1 for still using format_string, that is so oldschool.

tim.plunkett’s picture

FileSize
54.67 KB

format_string isn't touched by any lines of the patch that aren't deletions. Just that interdiff.
Rerolled for #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

OH I see, this interdiff was confusing.

catch’s picture

Assigned: tim.plunkett » alexpott

On the one hand this is an API change we don't have to do.

On the other hand the number of consumers of filter format APIs is pretty minimal, moving over to Alex for a second opinion.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +needs profiling, +Approved API change

I like the change as we're removing a lot of unnecessary code and making use of our nice new APIs which is a good thing.

However, filter_formats gets called a lot... we should profile the change as we're dropping a cache. We are ensuring that every time we call this function for the first time during the request we are going to have to filter the formats by status, order them and localise them. It should still load them from the config cache.

I think it might be okay to just add the cache back in.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -needs profiling
FileSize
1.54 KB
54.96 KB

Let's just do that.

tim.plunkett’s picture

Marked #2061885: Remove calls to deprecated global $user in filter module a duplicate, since this handles all of those cases.

tim.plunkett’s picture

#51: filter-2012312-51.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +Approved API change

The last submitted patch, filter-2012312-51.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
55.92 KB
1.34 KB

One call to filter_format_load() got added recently, glad I retested

tim.plunkett’s picture

FileSize
55.8 KB

Rerolled for the Plugin/Core/Entity move

tim.plunkett’s picture

#56: filter-2012312-56.patch queued for re-testing.

tim.plunkett’s picture

FileSize
2.88 KB

For easier reviewing, here's the interdiff since last RTBC.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #51 addressed the issues in #50.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Yet another reroll

git ac https://drupal.org/files/filter-2012312-56.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 57138  100 57138    0     0  19051      0  0:00:02  0:00:02 --:--:-- 24670
error: patch failed: core/modules/filter/filter.services.yml:10
error: core/modules/filter/filter.services.yml: patch does not apply
tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
2.79 KB
55.42 KB

Rerolled.

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -Approved API change

The last submitted patch, filter-2012312-61.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#61: filter-2012312-61.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +Approved API change

The last submitted patch, filter-2012312-61.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
55.68 KB
931 bytes

I got a little ambitious with my reroll.
It's insane to be have this sort of logic in hook_permission() anyway, and I really don't know if its worth all of this to get the links in the perm names.

tim.plunkett’s picture

FileSize
1.53 KB
55.27 KB

Per discussion on IRC, let's just skip that check altogether.

Status: Needs review » Needs work

The last submitted patch, filter-2012312-66.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
56.01 KB
759 bytes

Status: Needs review » Needs work

The last submitted patch, filter-2012312-68.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
54.29 KB

Since the global $user stuff is in such a state of flux, removing that. It's just not usable very early in the request, and filter is needed then.

Status: Needs review » Needs work
Issue tags: -API change, -Approved API change

The last submitted patch, filter-2012312-70.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#70: filter-2012312-70.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, filter-2012312-70.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Approved API change

#70: filter-2012312-70.patch queued for re-testing.

Eric_A’s picture

       // Only link to the text format configuration page if the user who is
       // viewing this will have access to that page.
-      $format_name_replacement = user_access('administer filters') ? l($format->name, 'admin/config/content/formats/manage/' . $format->format) : drupal_placeholder($format->name);
+      $format_name_replacement = l($format->label(), 'admin/config/content/formats/manage/' . $format->id());

The inline comment should be changed or removed, now that the linking is unconditional, right?

tim.plunkett’s picture

FileSize
932 bytes
54.29 KB

Very good point.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Yet another reroll...

git ac https://drupal.org/files/filter-2012312-76.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 55598  100 55598    0     0  29637      0  0:00:01  0:00:01 --:--:-- 33839
error: patch failed: core/modules/filter/lib/Drupal/filter/Access/FilterAccessCheck.php:1
error: core/modules/filter/lib/Drupal/filter/Access/FilterAccessCheck.php: patch does not apply
error: patch failed: core/modules/filter/lib/Drupal/filter/Access/FormatDisableCheck.php:24
error: core/modules/filter/lib/Drupal/filter/Access/FormatDisableCheck.php: patch does not apply
tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
55.17 KB

:(

The static::ALLOW issue changed a file I was deleting.

alexpott’s picture

Title: Remove legacy code from filter.module » Change notice: Remove legacy code from filter.module
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed a8d4542 and pushed to 8.x. Thanks!

alexpott’s picture

Issue summary: View changes

Update issue summary

catch’s picture

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Writing a change notice.

tim.plunkett’s picture

Title: Change notice: Remove legacy code from filter.module » Remove legacy code from filter.module
Assigned: tim.plunkett » Unassigned
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

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

Anonymous’s picture

Issue summary: View changes

updated issue summary