Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#79 | filter-2012312-79.patch | 55.17 KB | tim.plunkett |
#76 | filter-2012312-76.patch | 54.29 KB | tim.plunkett |
#76 | interdiff.txt | 932 bytes | tim.plunkett |
#70 | filter-2012312-70.patch | 54.29 KB | tim.plunkett |
#70 | interdiff.txt | 2.21 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettLet's see if this passes
Comment #2
tim.plunkettxjm suggested expanding the scope to include some of the other functions.
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.
Comment #4
tim.plunkettLeft in a stale service definition.
Comment #6
tim.plunkettWorkaround since global $user is not always (ever?) an instance of UserInterface.
Comment #8
tim.plunkettComment #9
tim.plunkettAh, 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.
Comment #12
tim.plunkettFixed two more bugs.
Comment #16
tim.plunkettOkay, finally fixed those.
Comment #19
BerdirHm.
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.
Comment #20
tim.plunkettIt's only used in
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?
Comment #22
tim.plunkett#20: filter-2012312-20.patch queued for re-testing.
Comment #24
tim.plunkettUm. I have no idea. I liked the patch in #16 better.
Comment #25
BerdirThat error usually happens if the permission name can't be looked up but I'm not sure why that would happen with those changes.
Comment #26
tim.plunkettThis is a reroll of #16 for now.
Comment #28
tim.plunkettFixed most of that.
Comment #29
tim.plunkettParts of this will go in as part of #2030129: FilterFormat has no access controller
Comment #30
tim.plunkettThat went in.
Comment #32
tim.plunkettMissed a spot.
Comment #34
tim.plunkettRerolled.
Comment #36
tim.plunkettWhoops, too aggressive
Comment #37
tim.plunkettRerolled.
Comment #38
jibranThis class needs some injection but I think it is out of scope. Maybe follow up.
Can we use
Drupal::request()->attributes->get('account')->id()
here?$account->hasPermission()
can be used here.Comment #39
tim.plunkettEditor module has almost nothing injected, and that class alone has 3 calls to \Drupal already.
I fixed everything else.
Comment #40
jibranIt 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.
Comment #41
dawehnerI seriously still scares me to use user_access in hook_permission
I guess we should use $account->hasPermission and String::placeholder now.
Just wondering why anything calls the method with an invalid parameter...
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.
Comment #43
tim.plunkettAs part of this conversion, we're replacing
filter_list_formats($format->format)
with$format->filters()
If I update the one test:
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.
Comment #45
tim.plunkettThis is getting ridiculous
Comment #46
dawehner-1 for still using format_string, that is so oldschool.
Comment #47
tim.plunkettformat_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
Comment #48
dawehnerOH I see, this interdiff was confusing.
Comment #49
catchOn 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.
Comment #50
alexpottI 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.
Comment #51
tim.plunkettLet's just do that.
Comment #52
tim.plunkettMarked #2061885: Remove calls to deprecated global $user in filter module a duplicate, since this handles all of those cases.
Comment #53
tim.plunkett#51: filter-2012312-51.patch queued for re-testing.
Comment #55
tim.plunkettOne call to filter_format_load() got added recently, glad I retested
Comment #56
tim.plunkettRerolled for the Plugin/Core/Entity move
Comment #57
tim.plunkett#56: filter-2012312-56.patch queued for re-testing.
Comment #58
tim.plunkettFor easier reviewing, here's the interdiff since last RTBC.
Comment #59
jibranBack to RTBC as #51 addressed the issues in #50.
Comment #60
alexpottYet another reroll
Comment #61
tim.plunkettRerolled.
Comment #63
tim.plunkett#61: filter-2012312-61.patch queued for re-testing.
Comment #65
tim.plunkettI 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.
Comment #66
tim.plunkettPer discussion on IRC, let's just skip that check altogether.
Comment #68
tim.plunkett#1758622: Provide the options list of an entity field broke this.
Comment #70
tim.plunkettSince 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.
Comment #72
tim.plunkett#70: filter-2012312-70.patch queued for re-testing.
Comment #74
tim.plunkett#70: filter-2012312-70.patch queued for re-testing.
Comment #75
Eric_A CreditAttribution: Eric_A commentedThe inline comment should be changed or removed, now that the linking is unconditional, right?
Comment #76
tim.plunkettVery good point.
Comment #77
benjy CreditAttribution: benjy commentedLooks good.
Comment #78
alexpottYet another reroll...
Comment #79
tim.plunkett:(
The static::ALLOW issue changed a file I was deleting.
Comment #80
alexpottCommitted a8d4542 and pushed to 8.x. Thanks!
Comment #80.0
alexpottUpdate issue summary
Comment #81
catch#2083415: [META] Write up all outstanding change notices before release
Comment #82
tim.plunkettWriting a change notice.
Comment #83
tim.plunkettPosted https://drupal.org/node/2083591
Comment #84.0
(not verified) CreditAttribution: commentedupdated issue summary