Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#50 | filter-1987124-50.patch | 66.2 KB | tim.plunkett |
#50 | interdiff.txt | 9.58 KB | tim.plunkett |
#46 | filter-1987124-46.patch | 64.21 KB | tim.plunkett |
#46 | interdiff.txt | 804 bytes | tim.plunkett |
#44 | filter-1987124-44.patch | 64.01 KB | tim.plunkett |
Comments
Comment #1
h3rj4n CreditAttribution: h3rj4n commentedAnd the patch. See if this one doesn't fail ;)
Comment #3
tim.plunkettBlocks #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
Comment #4
h3rj4n CreditAttribution: h3rj4n commentedAfter 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:
This way the url
formats/all
andformats/basic_html
both works. Replacing {notexisting} withadd
will break this.Comment #6
tim.plunkettSo we just need to fix the paths.
However, these should be EntityFormController subclasses and use _entity_form.
Comment #7
h3rj4n CreditAttribution: h3rj4n commentedLets test it.
Comment #9
h3rj4n CreditAttribution: h3rj4n commentedThe routing.yml contained the wrong route. It should be
/admin/config/content/formats/add
. Otherwise it isn't consistent.Added the patch.
Comment #11
tim.plunkettFilter plugins went in.
Comment #12
tim.plunkettToo much changed for a proper interdiff, I just diffed the patches.
Comment #14
tim.plunkettWell, 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.
Comment #16
tim.plunkettrerolled
Comment #18
tim.plunkettSloppy reroll.
Still seeing the problem described in #14.
EDIT:
Steps to reproduce:
editor_form_filter_format_form_alter() and editor_form_filter_admin_format_submit() are the problems.
Comment #20
tim.plunkettdawehner is my hero. He found the bug!
Comment #21
dawehnerI like this pattern of making it easy to place form elements between the existing ones.
another check plain.
Another injection.
I guess it just makes sense for all machine names?
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.
Just a nitpick: config could be injected.
check_plain now should be replaced by String::checkPlain
Comment #22
tim.plunkettReplaced/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.
Comment #24
Gábor HojtsyWoot! 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).
Comment #25
tim.plunkettGreat, a full review would be nice now that this patch should be green.
Comment #26
jibranSome minors
@param missing.
doc required.
Comment #28
dawehnerAren't we going with uppercase now?
One problem I had when manually trying out the patch is the following:
Comment #29
tim.plunkettI 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.
Comment #30
dawehnerThe addition to config EQ is perfect. The validation now works as expected, so this is ready to fly.
Comment #32
tim.plunkettSilly mistake. Yay test coverage.
Comment #33
dawehnerWow I wouldn't have expected that filter_format_load is not just a simple wrapper.
Comment #34
tim.plunkettYep, trying to fix that in #2012312: Remove legacy code from filter.module
Comment #36
tim.plunkett#32: filter-1987124-32.patch queued for re-testing.
Comment #37
tim.plunkettRandom failure from #2004408: Allow modules to alter the result of EntityListController::getOperations?
Comment #38
alexpottThis 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.
This can use the injected config... all this
filter_fallback_format()
does isreturn config('filter.settings')->get('fallback_format')
Comment #39
dawehnerHere is a longer explanation.
Comment #40
Gábor HojtsyLooks like great updates based on what @alexpott requested.
Comment #41
alexpottCommitted af5478b and pushed to 8.x. Thanks!
Comment #42
alexpottThis broke head :(
Comment #43
alexpottReverted a9b5c81 and pushed to 8.x.
Comment #44
tim.plunkettThis 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.
Comment #46
tim.plunkettNot 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.
Comment #47
kfritscheSorry 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.
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.
Comment #48
Gábor HojtsyComment #49
tim.plunkettThat's a good idea, and the code will become much more readable. Doing so now.
Comment #50
tim.plunkettOkay, that's done now. This is almost identical to RoleListController, which is a good sign.
Comment #51
tim.plunkettNote: 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.
Comment #52
kfritscheThanks 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.
Comment #53
Gábor HojtsyThe changes look good on visual review! Great refactoring.
Comment #54
Gábor HojtsyComment #55
alexpottCommitted 93ea9a4 and pushed to 8.x. Thanks!
Comment #56
Gábor HojtsyYayay! This makes config translation work for filter formats easily now. We can remove lots of ugly code from there. Yay!
Comment #57
YesCT CreditAttribution: YesCT commenteda 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.