- filter_format_load() should return a fully populated $format object -- including filters.
- A $format object should always contain all filters, enabled and disabled ones.
- With #11218: Allow default text formats per role, and integrate text format permissions, also filter_format_load() uses filter_formats() and thereby that should return fully populated $format objects. That looks utterly wrong, btw.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | drupal.filter-cleanup.16.patch | 12.39 KB | sun |
| #14 | drupal.filter-cleanup.14.patch | 11.63 KB | David_Rothstein |
| #13 | drupal.filter-cleanup.13.patch | 9.59 KB | sun |
| #12 | drupal.filter-cleanup-status.12.patch | 2.61 KB | sun |
| #6 | drupal.filter-cleanup.6.patch | 30.74 KB | sun |
Comments
Comment #1
sun- Some filter functions expect a text format id, not an object, but the function argument is $format. Rename those to $format_id.
Comment #2
sunNote that point 2) in the OP is now covered in #562694: Text formats should throw an error and refuse to process if a plugin goes missing.
Additionally, let's also account for #573856: Bring consistency into whether $filter is an array or object in here:
In filter_format_save(), $filter is an array.
In filter_list_format(), $filter is an object.
Comment #3
sunTagging absolutely critical clean-ups for D7. Do not touch this tag.
Comment #4
sun- Renamed $format to $format_id according to variable type.
- filter.test: Renamed $this->getFilter() to getFormat().
- Moved filter tips to filter implementations.
- Fixed coding-style of HTML filter tips.
Comment #6
sunMerged required changes from #562694: Text formats should throw an error and refuse to process if a plugin goes missing into this patch.
Comment #7
chx commentedAfter a through review, I think this is good to go. Also, I kinda trust Sun to be able to copy-paste a big chunk of code without hiccups.
Comment #8
dries commentedPart of this patch is still being discussed at #562694: Text formats should throw an error and refuse to process if a plugin goes missing. As I said in #562694, while I'm cool with this patch, I'd like to get a bit more feedback from other people. Either way, I'm very confident that we'll get this patch in before the end of Code Slush. Worry not, because it will get committed. :)
Comment #9
sun@Dries: no no no, this patch only contains the API clean-ups - there are no security-relevant changes at all. It will make that other patch smaller, because all API changes have been moved in here.
When this patch is in, we roughly have 2 FilterSystemRevamp issues left, from which only one is a highly critical API change (the other one is just a bug).
Comment #10
dries commentedAh, you're right. Sorry for not getting that. :)
Committed "ze patch" (french for "the patch")!
Comment #11
David_Rothstein commentedHm, the changes to filter_list_format() seem to have broken a number of things.
Since that function now returns all filters, you can get disabled filters showing up all over the place where they aren't supposed to.
Steps to reproduce: On a fresh install, go to the text format page and resave one of the existing formats. This calls filter_format_save() which puts all filters, including disabled ones, into the database (the problem doesn't show up on a fresh install immediately because the Drupal installer uses direct database queries for some reason - separate issue)... Then, go to a variety of pages (the "Configure" or "Rearrange" tabs, or create a new node and look at the filter tips) and you will see the disabled filters for that format showing there as if they were enabled.
It's not hard to fix, but I'm trying to make sure I understand the rationale for this API change in the first place. In most cases, when you call this function, you only care about enabled filters - but now we're forcing callers to get back all filters and be responsible themselves for ignoring the disabled ones? I do understand why we want a $format object to always contain all filters, but in terms of other uses of the filter_list_format() function, it seems like this change is not always as good.
Comment #12
sunSorry, not sure how I missed those when merging the other patch in here.
The rationale behind this change was that not only #562694: Text formats should throw an error and refuse to process if a plugin goes missing will require it (and there would be no other way to get that API change in after API freeze), and this API functionality is only supposed to be used by Filter module itself or other edge-case implementations that can deal with it, and it allowed us to remove some ugly logic from filter_list_format() for conditionally excluding disabled filters.
Listing filters for a format always should plain list all filters. Whether disabled or not. It's the job of the caller to figure out what to do with the data.
Comment #13
sunActually, there was more to it. The entire filter configuration and re-ordering form interactions did not have any tests at all, and one of the other FilterSystemRevamp patches introduced a bug, which lead to duplication of the settings of one filter in the settings of subsequent filters.
Fixed in this patch. And added tests to ensure we don't break all of this again.
Please note that those configuration/re-ordering tabs (rather intentionally) didn't see much love, because all of us intended to revamp the entire UI in #558666: UX/security: Revamp text format/filter configuration, which would have removed those horrible configure/order tabs. Not sure whether Dries and webchick will still allow for that major UX improvement (because it removes some forms, which counts as API change), but aside from that, I think the API is now prepared for the envisioned UI over there.
Comment #14
David_Rothstein commentedI reviewed the patch and it looks good to me... however, I noticed there were still a few places where filter_list_format() was being called with the second parameter that needed to be updated, plus an instance in text.module that still wasn't checking if a filter returned by this function was enabled, so I've fixed those in the attached patch as well.
Regarding the overall API change, yeah, I definitely agree that the $included_disabled parameter was ugly and nice to see it removed, but it also seems like most places that call this function really don't need the disabled filters (this is compounded by UI issues, because we do not distinguish anywhere the difference between a filter that is associated with a format but "disabled" vs one that is not associated with a format at all - so for any format that has been saved in the admin UI, the "disabled" filters in fact include every single filter on the site...)
Ideally, I think there'd be two API functions here, something like filter_format_list_filters() and filter_format_list_enabled_filters(). Not sure if we can squeeze that in past the API "freeze" or not?
If not, I think we should at least put in some improved PHP Doc for filter_list_format() that explains in more detail that it returns disabled filters as well as enabled ones.
Comment #16
sunJust added the piece of PHPDoc David suggested.
Comment #17
dries commentedCommitted to CVS HEAD. Thanks.