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.
Problem/Motivation
Accept headers are a problem, due to its related caching problems, see #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use for more information.
We use accept headers both for routing, but also for a way to convert the result of the current controller into a different format
like a modal, dialog or ajax.
Proposed resolution
- Add _wrapper_format as query parameter instead of using an accept header
- Use that query parameter in order to convert the response into the modal/dialog/ajax.
- Convert the various tests to use query parameters ...
Remaining tasks
User interface changes
API changes
Add a _wrapper_format, which replaces the accept headers.
Comment | File | Size | Author |
---|---|---|---|
#134 | interdiff.txt | 8.05 KB | dawehner |
#134 | 2472323-134.patch | 30.86 KB | dawehner |
#128 | interdiff.txt | 3.24 KB | dawehner |
#128 | 2472323-128.patch | 30.85 KB | dawehner |
#122 | interdiff.txt | 654 bytes | dawehner |
Comments
Comment #1
dawehnerComment #2
nod_Everything using the ajax API is already always sending
{js: true}
in the data (on top of the jQuery-specific header:X-Requested-With
).The patch makes the frontend send an option for the modal properly, it's called
data-dialog-type
in the HTML attribute anddialogType
in the data sent to be backend. We might want to rename itdata-drupal-ajax-type
and make the api more consistent (we would be getting rid of the magicuse-ajax
anduse-ajax-submit
classes). but it's a start.I have not touched the backend stuff so all the negociation stuff needs cleanup and it needs to use the new stuff.
Comment #3
larowlanDamn, but get it
Comment #4
dawehnerLet me give it some try.
Comment #5
dawehnerHere is a quick intermediate patch.
Comment #6
dawehnertests ... please tests
Comment #9
dawehnerThere we go.
Comment #10
dawehnerHere are some fixes ...
Comment #11
pwolanin CreditAttribution: pwolanin at Acquia commentedLooks basically fine - might need to check for an existing query string in the JS
Comment #12
dawehnerAdapted the javascript according to peter.
Comment #13
pwolanin CreditAttribution: pwolanin at Acquia commentedin the if/else you don't really need an extra variable, how about jut:
Also - it would probably be a best practice to urlencode element_settings.dialogType if it coule be specified by a module to some random value?
Comment #14
dawehnerGood point, lets do that.
Comment #16
pwolanin CreditAttribution: pwolanin at Acquia commentedShould we have a whitelist here? Or at least verify it's sane like
[a-z0-9_]
Comment #17
larowlan+1 to a white list, container parameter?
Comment #18
pwolanin CreditAttribution: pwolanin at Acquia commentedShould we have a whitelist here? Or at least verify it's sane like
[a-z0-9]
?Allowing raw user input to pass into the system here seems like something that could cause security issues later.
Comment #19
neclimdulRather then messing with the content negotiation class why don't we just make this its own request subscriber?
Comment #20
Crell CreditAttribution: Crell at Palantir.net commentedComment #21
Crell CreditAttribution: Crell at Palantir.net commentedPer the phone summit today, should we use a more generic name than dialogType? wrapper, envelope, etc?
The patch looks fine to me otherwise.
Comment #22
dawehnerGood point, I'll go with that tomorrow morning and maybe just go with _format for now?
Comment #23
neclimdulMy gut says we should not use _format but I'm not really sure. Naming is hard but easily refactored so we can keep discussing it.
Comment #24
dawehner\Drupal\Core\EventSubscriber\WrapperFormatSubscriber
Comment #26
dawehneryeah our error reporting is not perfect
Comment #27
Crell CreditAttribution: Crell at Palantir.net commentedGood call on the constant. :-) Though if we expect people to use it, putting it outside of the subscriber would be a good idea. (Not sure off hand where else we'd put it, though.)
This is probably fine for now, but we may have to split it to its own property later. Which of course we're trying to avoid having more of. Carp.
Comment #28
dawehnerDo you think is really something we have to / should do? It feels just entirely wrong, if I'm honest.
Can you elaborate why you think a split is needed? I still think its a pure internal detail whether a specific format is used (or not) for routing / as a controller wrapper.
Comment #29
Crell CreditAttribution: Crell at Palantir.net commentedIt's been my experience that "using one thing for two things" backfires more often than it works out. SRP is hard but it really is the driving force behind good software.
Can we guarantee that we will never ever ever need to differentiate between the payload format and envelope format? We will always care about only one or the other? I'd rather be cautious and give ourselves an out.
Comment #30
webchickWe talked about this being a blocker to #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use
Comment #31
larowlanShould we have some sort of whitelist here? could be container parameter to allow others to edit/add to it. Raw user input this deep in routing feels wrong for some reason.
Comment #32
dawehnerThat is a good question.
Well it seemed to be that we want to store them different anyway, see #29, but I think in case we have a whitelist of them stored somewhere, we could
use the request format and differentiate the behaviour based upon that stored values.
Comment #33
Wim LeersThis patch looks like it's almost ready!
#31++
Is this now a generic concept? Or does it really only apply to HTML responses? Can we think of use cases of this concept for e.g. JSON?
Comment #34
dawehnerI can't really think of something else, but well, you never know. At the moment I went with _wrapper_format as it did explain what it is used for.
Comment #35
Wim LeersThen that's fine, I think. But if it were HTML-specific, then we could write clearer docs. That's why I was asking.
NW for #31, then, and then this should be ready!
Comment #36
neclimdulSeem this should just be in MainContentViewSubscriber. Its only job is already just to do this sort of envelope resolution.
Manually testing, I can't add a block instance. I assume these not matching is the reason why.
Comment #37
dawehnerThis is tricky. For a bit I thought about just using
'%main_content_renderers%'
as this contains exactly those bits, BUT,in case we whitelist them here, don't we effectly bypass the checking which already exists in
\Drupal\Core\EventSubscriber\MainContentViewSubscriber
and helps in terms of DX due to its nice exception. In other words, you can't just something random and it will be not validated.
The other more complex question is the one from crell in #29. Do we want to treat those wrapper formats internally as the same or not.
In case we don't we might have to adapt the way how we render links for both
_format
as well as_wrapper_format
, which would be kinda sad.PS: just sad the comment from neclimdul.
Comment #38
dawehnerWell, the question is whether we want to take into account this query parameter also when we render URLs, because in case we do, we should not rely on the magicness
of the queyr parameter in more than one place.
Mh, I manually tested mostly views, indeed its broken, here is a fix for that.
Comment #39
Wim LeersNo, it only does that for controllers that return a render array. This is why I asked #33: if it's intended to be more generic than only for HTML-like things, then it needs to be separate. If it's only for HTML-like things, then yes, it should be in
MainContentViewSubscriber
.Comment #40
tim.plunkettNot sure if this is a temporary change, but if it staying, this should just check with ===
Beautiful!
Comment #41
neclimdulTook a stab at fixing a couple things. There where a lot of tests that weren't converted. I assume more will fail when I upload this.
I still don't understand why MainContentViewSubscriber isn't the place this is suppose to happen. Moving it though helped me expose a bunch of places that where still relying on content negotiation where it shouldn't so that's in the patch.
There are still a bunch of tests that have our vendor accept headers so we'll need to fix those before this can go in. As I mentioned I expect some of those will break in testbot.
Comment #43
Wim Leers#41: can you please provide an interdiff?
Comment #44
dawehnerChanged two criticals parts.
a) For a case where application/hal_json is sent the envelope should be NULL, so we should not fallback to HTML
At the same time we have to
b) It doesn't make sense for now to change drupalPostAjaxForm IMHO, given that its a POST request, which won't suffer in the parent issue.
Comment #45
Wim LeersHuh, an Accept header again? I'm confused.
Comment #46
neclimdulSorry, I have the interdiff I just didn't get it on the upload.
No, these where what I was fixing. These methods are suppose to be used to test our ajax/modal/dialog code and they're still using accept headers. If we don't fix them, we're not testing the code we are changing and the passes are false impressions of the actual state of things. @see earlier review where things where passing but not working.
Comment #47
neclimdulthis really doesn't want to make it onto the issue.
Comment #48
dawehnerWell, this code though is also used in order to to test the upload functionality which breaks with the patch as it is. I'm totally fine with reverting the change, but I have't figured out why that change actually matters. file/upload returns an AjaxResponse anyway, so the envelope should't matter, but for some reason it does, investigating a bit more.
Comment #49
dawehnerAlright, this time we parse the URL correctly, so we don't run into url encoding issues.
Comment #50
neclimdulSo I didn't get this but after spending a while with it, it does something very specific and non-obvious. The difference in these lines is _the_ way we provide a 406 for content negotiation. This should be handled in a more specific and clear way I think but this is more the focus of the parent issue so I'm just going to document it as such and leave it so tests pass.
Also fix a failing test because of a string match and move URL encoding fix to only affect settings based "path" stuff which is where the problem was.
Comment #51
jibranAPI change section needs update. We also need change notice for this change.
Needs some doc love.
Comment #54
neclimdulBetter?
Comment #55
jibranCorrect.
Comment #56
neclimdul@todo statements don't parse on multiple lines.
Comment #57
jibranPlease see https://www.drupal.org/node/1354#todo
Comment #58
neclimduli stand corrected, phpdoc doesn't seem to and phpstorm doesn't but PSR6 allows it, I'll roll a new patch.
Comment #59
neclimdulComment #60
jibranThanks for the fix.
So we have a constant in PHP. Can we have some kind of key on drupalSettings which we pass from PHP so that we can change it from one single place in future? Just an idea.
Comment #61
Wim LeersSending the value of a (by definition hardcoded) constant over via drupalSettings means sending it with every request. That's not okay.
I think we can handle renaming that line of code in the JS.
Comment #62
jibranso something like
var WRAPPER_ENVELOPE = '_wrapper_format';
then?Comment #63
Wim Leers#62: that'd be fine, yes.
Needs to be documented.
Also: why the leading underscore?
s/requestFormat()/getRequestFormat()/
Note that this comment is inside the
is_array($result)
if-test, which checks if the controller result is a render array.So I don't see how this is "a fallback method".
It's quite possible this is only confusing because the code flow *elsewhere* is confusing, but at least this class was very clear, very focused.
This makes it vague, IMO.
This is definitely a bad change.
This view subscriber is about render a render array in the requested format. Yes, in core there are only html/modal/dialog/ajax, which are all variations on "html". But it's perfectly possible that contrib adds more formats.
That's why this code is sending a 406 response in case a format is requested that we cannot render the render array into, per the HTTP spec. This code written to carefully follow the HTTP spec. These changes break that.
If this code is indeed only about wrapper formats (which I'm not at all sure about), then this does not care about Accept headers, and then this should also never send a 406; sending a 406 would be up to the rest of the system then. i.e. the entire else-branch should then be removed.
This is why I find these changes so confusing & conflicting:
- on the one hand, we say this only cares about wrapper formats, which can only be set through query parameters
- yet on the other hand, we send a machine-readable 406, which is actually only applicable to Accept-header content negotiation; it'd make more sense to fall back to HTML in this case
- and finally, this makes it impossible to do content negotiation to other formats than html/ajax/modal/dialog, i.e. this is implicitly claiming "render arrays only ever need wrapper formats, never need 'proper' formats, because render arrays will always render to HTML"
Why do we need to retain the original wrapper?
If there is a valid reason, then let's do
and hence not do this dance?
Comment #64
dawehnerWell, we want to indicate that its not about the content of the page but more the wrapper, which is more of an internal value.
Not sure what to do here ... I prefered the additiona lsubscriber to be honest.
Well, the problem is that this patch simply doesn't solve all the usages of accept headers, of which one is: you request /entity/1 with application/hal+json without hal being enabled. This is the place here in HEAD which throws the 406 and will continue to do so ...
Note: We just trigger the code in case we have no matching route, which won't happen if you specify the extension / query format or even accept headers.
Have a look at the code ... If you pass
$path
it first goes to the page with the form you want to post via ajax. This page should be rendered using the standard HTML techniques. That path though should also support$options
Comment #65
Wim LeersNote that removing the else-statement would allow us to handle it elsewhere. Not sure what makes most sense.
s/Url/URL/
80 cols
The name indicates genericness. Yet it lives in
MainContentViewSubscriber
, which only cares about render arrays.So it's not actually generic…
Therefore, +1 to:
Comment #66
dawehnerWell, the question is, where we store this value. Previous versions of the patch stored it in
$request->getRequestFormat() but there seems to be agreement (not sure personally) that those kind of formats are a different thing.
Comment #67
neclimdulOn the question of the fallback method, that is infact what it does. You are correct, the code elsewhere is unclear. I agree it should be a separate subscriber but we're about to replace logic with something in the parent issue so I just added the @todo. I assumed it out of scope as well.
To explain the code flow, Accept header format matching is done by the router. rest.module adds a new route for each format and then lets the router resolve that and then serializes it. That serialization then isn't an array so it skips the code in this subscriber.
If it fails to route based on getRequestFormat() though the default route(our HTML route) gets used and it hits this code. Now, here is where all the assumptions that really make this confusing come in. Because there isn't a response or a string (array check) we know this is a Drupal centric return and we know we're rendering HTML(or maybe one of the special wrapper formats). Now, the request format doesn't match the wrapper list so we know its not html(html is always in the mime types) and so we assume the format was invalid and serve a fallback 406.
If you're still confused, I don't blame you I spent an hour playing with it and trying to figure out why rest.module tests where failing before it clicked.
Comment #68
neclimdul63.4) mime_types -> wrapper_envelope - This matches terminology and is technically more accurate I think because we are not using mime-types for negotation. The error implies you supplied the wrong mime type.
That said... that's only for this issue. In the fallback it is incorrect and a bad change, I'll see if I can tease apart the two use cases.
Comment #69
Wim Leers#68: But
MainContentViewSubscriber
only does something for controllers that have returned an array. The REST module's controller isRequestHandler::handle()
. And looking at that code, I see only 3 ways to return:Response
object (with HTTP status 400). Therefore,MainContentViewSubscriber
cannot possibly be called.Response
object. Therefore,MainContentViewSubscriber
cannot possibly be called.$response
value retrieved during the invocation of the operation on the resource plugin, and before returning it, we always do$data = $response->getResponseData()
, which would cause a PHP error if$response
were an array.In other words: I don't see how the code flow could ever allow for a REST module response to be an array. :( Did I make a mistake in this analysis?
Comment #70
dawehnerSo the setup is the following:
Hal module is disabled.
Then we request entity/123 with application/hal_json ... technically then the normal HTML controller is called, aka. a render array is returned, but still
we wanted to return hal_json, which is not a right wrapper envelope. As a reminder, accept headers are optional, which is one reason why those cache poising exists.
Comment #71
Wim LeersAha! dawehner++
So the problem is that:
KernelEvents::View
subscriber (i.e. N formats => 1 controller + VIEW subscriber).I guess that if we'd be consistent (either always approach 1, or always approach 2 — I'm not judging which is better), then we wouldn't have this problem. But there's no changing any of that right now, obviously.
Alright, so with that better understanding, reviewing the patch again:
Then this definitely does not make sense. If we separate "wrapper envelopes" from "formats", then it's also not okay to fall back to the request format in case no wrapper envelope is specified. I.e. we should always fall back to "html", then.
And then this does indeed make sense.
to
i.e. to ensure that the main content view subscriber only ever cares about the "HTML" request format. Because that's what we are saying here, that conceptually, this is always about HTML, and it just may be wrapped differently.
… but that poses two problems:
In conclusion, I can't help but think that the the two distinct ways of handling "send a resource in a certain format" as described at the very beginning of this comment is causing all this trouble: it forces us to invent the concept of "wrapper envelopes", which then causes further problems because "wrapper envelopes" implies that the response of "regular routes" (i.e. "non-REST routes") is always of a single format — otherwise we wouldn't know how to reliably wrap it.
Why did we switch from the separate subscriber approach that we had until #38 to this approach? I think it's because @neclimdul said this in #36:
But that's not completely true; it just happens that Drupal core doesn't have anything that turns a render array into something not-at-all-HTML-like.
However, let's assume that it would always be HTML-like (which could indeed be argued). What we've now ended up with is two levels of content negotiation: base types (HTML) and more specific subtypes (html/dialog/modal/ajax), if you will.
Which could be fine, except that we're actively relying on very twisted behavior due to those two distinct ways again (top of comment): one type is missing (HAL-JSON), so the router decided to fall back to HTML, but the only accepted format is actually not supported, so we should send a 406, but we already fell back to HTML, so it shouldn't be
MainContentViewSubscriber
's job to figure out that we actually didn't request HTML, but HAL-JSON, even though we did fall back to HTML, etc.If that would be fixed in #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use, then fine, go ahead. But it is extremely confusing. And IMO it is because either Symfony, or we, or both, have failed at making a clear decision on how the same resource should be available in multiple formats (again, see the top of the comment).
Comment #72
Crell CreditAttribution: Crell at Palantir.net commentedI went back and forth on this a lot, but in the end I think they have to be separate. In some cases you want the same "content" but rendered differently; in others you want different logic to run depending on the format. As long as we have controllers that can execute arbitrary code I don't think we can unify that flow, and not having arbitrary code controllers is a Drupal 9-level change at least. :-)
In hindsight, a truly REST-centric system would not have controllers. It would only have resources. But given that HTML is about 20x as involved as any other format, even that unification would be difficult to do. I've toyed briefly with this area on my own and don't have a good solution given the use cases that Drupal tries to support (most of which are entirely legitimate).
That said:
We keep saying this, but as long as there is are #markup, #prefix, and #suffix keys it will never be the case. Render arrays are an abstract syntax tree of the output, but that output is NOT, by that point, format agnostic. That's a lovely theory but to my knowledge has never been true. I long ago accepted that the theme and render systems were HTML-speciific, and that has made my life far easier.
Comment #73
Wim LeersExactly!
Indeed.
Indeed; it depends on how you look at it. A render array can be turned into a PDF just like HTML can: by taking the rendered HTML and putting that into a PDF.
This is why I said at the bottom (below the last horizontal rule) in the "However" and "Which" paragraphs that 1) I could see us choosing to make it so that "we can only render render arrays as HTML" but 2) that that then means the current patch's changes to
MainContentViewSubscriber
don't make sense. Could you read those two paragraphs again, and try to formulate what you think would be a sensible solution? :)Comment #74
catchMarking this as triaged since it blocks #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use which has already been triaged.
In response to #72/#73 are we just saying that view listeners are for formats that either are, or can contain, HTML? Both PDF and epub fall under that category, and I think we discussed on the other issue that for at least those two formats they could probably be implemented as either REST or via render arrays (i.e. potentially you could have competing contrib modules doing each approach).
Comment #75
neclimdulI very much see render arrays as only useful for rendering "HTML-like" content. It seems like we're all in agreement on that though. That has definitely driven a lot of my decisions and thinking about this and the parent issue.
I wonder if we're trying to over complicate something here though. Because someone chose to implement that weird clash of concerns in that subscriber shouldn't be leading this discussion. That's just an implementation detail we can sort out. Separating the check aligns with our short term goal of this issue and decouples things so we can continue to sort out the larger problem in the follow ups.
This patch:
Thoughts?
Comment #76
dawehnernitpick: needs docs or at least a visibility.
too bad, so we can't give any tips at that point anymore?
Comment #77
Wim LeersThis is the reason why I think this may be acceptable to land now, so that #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use is unblocked.
This can be simplified to:
Better comment:
Comment #78
dawehnerWell, right, under the assumption that no accept headers are sent, it will be determined to be HTML
Comment #79
neclimdulWe were only listing vendor formats for envelopes, not mime types for accept negotiation so it wasn't really relevant.
#77.3 yeah. agreed and Crell +1'd on IRC.
#77.4 No, it sadly can't be simplified. The reason is non-obvious though and I almost posted this with something similar but then I did some manual testing and ajax.js didn't work.
The reason its more complex is because if ?_wrapper_format is set we actually have to ignore the accept header format. This is because $.ajax is being given
Datatype:json
as an argument(totally valid) and so is adding theapplication/json
Accept header. If we only check getRequestFormat(), when we get to this code we see 'json' != 'html' and rather then using the wrapper, we skip it and fail through to the 406 code.All that said, we could probably revisit this when we start changing accept based content negotiation because having to explicitly opt in to accept based negotiation might change this requirement.
Comment #80
Wim LeersExactly.
Alright, let's get this one done, so that the more important critical is unblocked!
Comment #81
neclimdulNoted documentation fixes.
Comment #82
Wim LeersFound a bug during manual testing.
STR:
/node/add/article
/admin/structure/views/view/content
Comment #83
neclimdulOuch, yeah 2 separate javascript bugs too.
Comment #84
neclimdulwoops, correct interdiff
Comment #85
Crell CreditAttribution: Crell at Palantir.net commentedNo need to do this ourselves. Just throw the appropriate exception. I believe that should already get picked up properly.
Why can't we replace it yet?
Comment #86
Wim LeersConfirmed working. NW for #85.
Comment #87
kim.pepperFixes for #85
Didn't do anything here.
Comment #89
kim.pepperHmm. Looks like we *do* need to do something if removing AcceptNegotiation406.
I'm happy for someone else to take a look at this.
Comment #90
kim.pepperSpoke with @dawehner at the sprint, and it seems I misunderstood what @Crell was asking for in #85.
Re-added the listener, and now throwing the exception.
Comment #91
dawehnerThat looks reasonable!
Comment #93
kim.pepperForgot to re-add the service definition. Also added a Json exception handler for 406, but still getting a failure for matching the returned message, so I don't think it's getting called.
Comment #94
neclimdulDon't see failures with this. The exception definitely creates a different output as can be seen in the interdiff.
Interdiff vs #83
Comment #95
larowlanReview of #93
Needs a docblock
Can we avoid the $GLOBAL?
nit: this would be easier to read with [] array notation
Comment #96
larowlan1 and 2 of #95 apply to #94 as well.
Comment #98
dawehnerWell, I think this change is wrong. A 406 error is not a fatal error. We rather should have a custom exception message for 406s
Comment #99
BerdirEven if it's a 5xx, it's actually an uncatched exception or something like that, and never a fatal error. So that's highly confusing in all cases...
Just suggesting that this might be something that could be improved in a follow-up issue, since it's already bad :)
Comment #100
Crell CreditAttribution: Crell at Palantir.net commentedLooks like some code got lost between #93 and #94. Let's try to fix this up.
Comment #101
Crell CreditAttribution: Crell at Palantir.net commentedOne little word...
Comment #103
neclimdulThis doesn't seem relevant since the test isn't failing even though the response would be different.
Comment #104
Wim LeersWhy do we define the priority twice?
Hrm… why do we special-case JSON? Or must this exist for every format?
OTOH, this already exists for 403/404/405… so… I guess changing this is out of scope? Not sure.
s/Url/URL/
See #63.5.
Any reason we can't replace this yet?
Comment #105
neclimdulbah, I'll know those out.
Comment #106
Crell CreditAttribution: Crell at Palantir.net commentedI already am, stand by while I write a comment. :-)
Comment #107
Wim LeersNever mind #104.4, @neclimdul pointed out in IRC that @dawehner already answered this in #64.
Comment #108
Crell CreditAttribution: Crell at Palantir.net commentedOK, here's the issue. Turns out, core is not catching ANY hal_json errors. They're falling through to the default exception handler.
Also, the Json exception handler and the default handler use *different* properties for the error message. Because Durpal.
I talked briefly with dawehner and Alex Pott. Standardizing the properties is now a spinoff issue: #2486943: Standardize error message property name
Since hal_json is module-provided, not core provided, we put its error handling in its own module service but the behavior is identical. Then we fix the tests at least for now.
Re #104: All per-type, per-error is opt-in, so adding the method is the proper way to do it. And I have no idea why the priority is specified twice, so let's fix that.
I can't speak to Wim's other questions.
Comment #109
Wim LeersMissing space before closing curly brace.
"HAL" or "HAL JSON"?
Missing newline in between.
Comment #110
Crell CreditAttribution: Crell at Palantir.net commentedMeh.
Comment #111
neclimdulTook a stab at making #104.4 a little clearer anyways. there wasn't really any docs on what was happening. Also removed "todo" added that needed to go away.
Comment #112
Wim LeersSo much clearer, many thanks!
Final round of remarks. I'd RTBC this if it was just the first nitpick. The third should be fixed, but the second should *definitely* be fixed
Super nitty nit: s/Doc/doc/
Quoting #95.2:
The reason that this works, is that we can't run JS in our tests. So we have to mimic what the JS would do, in PHP. That's what
DialogTest
does. But, for correctness, this should actually still set'data-dialog-type => 'modal'
.Comment #113
Wim LeersAlso: I did another round of manual testing, still working perfectly :)
CR is still needed.
Comment #114
neclimdul1) sure.
2) actually, we sort of use GLOBALS about as often as
global $base_path
and this terrible code stands out for future improvements/refactor imho.3) sure.
Comment #115
neclimdulComment #116
dawehnerDon't complain, just add a follow up: #2487055: Replace base_path with $request->getBasePath()
Comment #117
Wim LeersLet's do this!
Next step: #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use.
Comment #118
dawehnerAre we sure this actually works? I would have expected to be added to the url itself.
Comment #119
kim.pepperWorked with @dawehner to fix the javascript dialogType.
Comment #121
dawehnerSo yeah that failure is simply caused by the fact that we look at the exact entries of arrays.
Comment #122
dawehnerLet's quickly fix stuff.
Comment #123
Wim LeersComment #124
catchGenerally this looks great. Just this made my eyes screw up a bit.
It's not clear to me why this much work is necessary for something that's explicitly set in ajax_settings. Could use a comment?
Comment #125
neclimdul@catch Good question, its really confusing but that's sort of par for the course in webtest I guess. When we're given path for the ajax url as an argument we get a site path like "node/1." You can't see it in the diff, but earlier in the method $ajax_settings however can be generated by xpath'ing form information out of a GET request so the url will have been prefixed with the site base_path. We go through all this trouble to remove the path so the rest of the code can work on the assumption that we have a simple site path.
Comment #126
catchOK that could really use some docs. Otherwise I don't see anything else to hold this up, looks good overall.
Comment #127
catchCNW for the docs.
Comment #128
dawehnerAdd some documentation and used some better code in ContentNegotation, thank you tim!
Comment #129
dawehnerImproved the issume summary, and a beta evaluation: https://www.drupal.org/node/2488192
Comment #130
tim.plunkettThese docs do help make sense of this ugliness
Might as well use the function, +1
Thanks for the updated issue summary and CR!
Comment #133
tim.plunkettRandom failure in Drupal\views\Tests\Plugin\RowRenderCacheTest->doTestRenderedOutput()
Comment #134
dawehner@alexpott, @xjm, @tim.plunkett talked about naming it WRAPPER_FORMAT, because honestly, this is a constant which I understand.
Comment #135
alexpottThis issue addresses a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed ad87128 and pushed to 8.0.x. Thanks!
Comment #138
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedPublished the change record ...
Comment #139
yched CreditAttribution: yched commentedI adjusted the CR to make the examples consistent (the request and render array examples were demonstrating dialogType 'modal', the #ajax examples was demonstrating dialogType 'ajax' - moved all to 'modal' for symmetry)
Comment #140
andypostFiled follow-up #2725435: Remove outdated @todo pointing to #2364011