Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
filter.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Feb 2011 at 10:47 UTC
Updated:
29 Jul 2014 at 19:21 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
agentrickardSimple patch.
Comment #3
sun#1: 1053518-format-add.patch queued for re-testing.
Comment #5
agentrickardI see no point to adding a test here.
Comment #7
sunThis bugs me since D7 and I have no idea why I didn't add a redirect there. Verifying this expectation in tests is a piece of cake though :)
However, we might want to wait for #1779026: Convert Text Formats to Configuration System, since I think the changes here conflict with that patch.
Comment #9
sunI had a chance to double-check the changes here against #1779026: Convert Text Formats to Configuration System tonight, and the two changes do not conflict with each other, so we can happily move on, once the test failures are fixed.
This patch will only conflict with the follow-up clean-up issue #1881664: Clean up and complete text format config entity conversion. Ideally, we get this patch in before work starts to happen over there.
Comment #10
agentrickardRe-reading the o.p., the tests break because the request was only to redirect when adding a new filter.
This patch adjusts that behavior and fixes the failing tests that result.
I believe the assumption is that updating a filter should return you to the filter form, so you can review changes. Adding a filter should take you to administer filters (or, alternately, to view the filter you just created.)
Comment #11
sunmmm, I don't think that makes sense though. The general form submission redirect pattern throughout core is to redirect to two discrete locations, depending on the thing you added/edited:
- for content/viewable entities, we redirect to the view.
- for configuration/unviewable entities, we redirect to the listing page.
We do not make a differentiation between add/edit currently.
The only exception to that rule is the taxonomy term add form, which redirects back to itself, but that's with the concrete idea and purpose that people who are using that form often want to add multiple terms subsequently. It's a relatively bold assumption though, and actually we should remove that special behavior and add a separate "Save and add another" submit button to the term-add form.
For the text format form, there's nothing to review after submitting. In fact, I think the lack of a redirect stems from the D6 user interface of text formats, which had the filter sorting + filter settings as separate tabs/pages and thus, that configuration was not available until a format was saved - hence, no redirect. That's no longer the case though.
Comment #12
agentrickardOK, then we redirect in both instances, and that will mean removing some of the assertFieldByName pieces in the current tests. (Or using the pattern of durpalGet in the later patch).
Comment #13
sunYeah, drupalGet() back to the form. We obviously don't want to remove test coverage. ;)
That said, the
assertUrl()changes from #7 should be taken over as well, since that's explicit test coverage for verifying that we end up at the expected URL after posting.Comment #14
agentrickardGot it.
Comment #15
agentrickardWith passing tests (locally).
Comment #16
sun#15: drupal8.filter-format-form-redirect.15.patch queued for re-testing.
Comment #17
sunGlad that it still passes. It's about time to fix this. :)
Comment #18
catchMakes sense. Committed/pushed to 8.x.