The goal of this issue is to introduce a new base architecture for managing and communicating out-of-band information, such as CSS/JS, via a custom PartialResponse class, and integrating that with our basic kernel request flow. This lays a major contextual foundation for the goals of SCOTCH.

Problem/Motivation

There are a couple reasons this issue is needed:

  • The current system is reliant on encapsulation-breaking globals (e.g., drupal_add_css(), drupal_add_html_head()) Request/Response model. This undermines testing (albeit in ways that are mostly taken care of by drupal_static()), but more importantly means that the outermost page html can be mutated anywhere, anytime. Caching, broken.
  • A class like this means that blocks, SCOTCH controllers, etc., all have one, encapsulated way in which they return HTML response instructions. This is conceptually similar to render arrays, but different in that they are more stable/predictable (it can be mutated, but in a much more traceable/structured way) and grokkable (b/c the functionality and docs can all live in one place).

Much of what this patch seeks to do is limit the mutability, and increase the introspectability, of top-level renderables. It is all built on the cornerstone idea that the page/html-level case is special enough to merit a slightly different approach than "just use render arrays."

Proposed resolution

The overall problem is being tackled in stages: this patch introduces the PartialResponse *only* with methods for capturing the main HTML string, and leaves the various other data (meta tags, head links, css/js assets, title, etc.) for follow-up issues. This issue gets our foot in the door, allowing the SCOTCH controller patch - #1812720: Implement the new panels-ish controller [it's a good purple] - to start doing its thing properly. By separating each of the other global functions/datum into their own issues, it lets us deal independently with the swathe of logic that needs to be updated when the global function is removed.

This patch also introduces a subscriber, the HtmlViewSubscriber, which is the salt to PartialResponse's pepper. The subscriber looks for a PartialResponse to come back from the controller, and if it does, it massages the partial response's data and calls theme('html'), then drops that into a full, proper Symfony Response. For now, there's very little massaging that happens. Once we add assets in, though, this subscriber (or possibly a standalone preceding one) will be the place where we trigger the asset prep, and that's fairly meaty.

There's a ton more that could be said about the general approach to the problem. A lot was here before, and is still in the revision history for this issue summary, if you care to see it.

Remaining tasks

A test or two needs to be written to ensure that the handoff of a PartialResponse through the HtmlViewSubscriber to theme('html') works correctly. With that, though, this patch *should* be ready to go in.

  • Kill asset globals (drupal_add_css() and drupal_add_js()) and shift management of assets onto the PartialResponse. (Blocked by #1762204: Introduce Assetic compatibility layer for core's internal handling of assets)
  • Kill drupal_add_html_head*() by disentangling it into its respective parts (meta tags, non-css link tags), updating the structure of html.tpl.php and shift them onto the PartialResponse.
  • Kill drupal_{s,g}et_title() and shift it onto the PartialResponse.
  • Shift html_attributes out of template_preprocess_html() and into HtmlViewSubscriber.

We probably don't need to shift drupal_add_http_header() to the PartialResponse; such things should probably be done with an event subscriber that operates directly on the final response via the later kernel event.

There's a bunch of changes that these will eventually entail for the html preprocess hooks - for example, $html_attributes can and therefore should be populated in a place where the container is properly injected. That's to be expected; we can deal with them issue by issue.

User interface changes

None.

API changes

It's the new API, but nothing has to be ported to it immediately - not until we get to the subtasks listed above.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sdboyer’s picture

Issue tags: +Stalking Crell

tagging for Crell

Anonymous’s picture

this looks awesome.

one thing that doesn't quite ring true though:

And since it's relatively easy to mock up a Request object in order to do so, it means we just need to load all routes, mock up a Request, and we can introspect all of the Responses produced across the entire system. Bam: full insight into real patterns of asset usage, systemwide.

sites can return different html/css/js for a given route based on user role, configuration of a module, day of the week, phase of the moon etc yeah? so this will likely not be as simple as you suggest.

Anonymous’s picture

Issue summary: View changes

fix list numbering

sdboyer’s picture

great :)

sites can return different html/css/js for a given route based on user role, configuration of a module, day of the week, phase of the moon etc yeah? so this will likely not be as simple as you suggest.

yeah, i wasn't quite clear about this. i'll expand the issue summary when i get a chance.

basically, this is what conditional plugins are for: a way of declaring exactly such variances explicitly. if, when registering an asset (or route), we can declare the conditions under which it occurs explicitly, then that information can be tracked. in which case it becomes possible to reflect on those explicitly declared conditions to determine those permutations.

don't know that we'll get to it for assets, but we really need it on routes.

sun’s picture

Overall, this sounds like a good summary of the parts we've discussed in various other issues up until now. Thanks, @sdboyer, for writing it up.

  1. Please keep the "arbitrary asset" feature of #attached, which allows it to be used for drupal_add_css(), drupal_add_css(), and drupal_add_library(), but also any kind of "add/build handler" for more sophisticated attached assets; e.g., drupal_add_tabledrag(), etc. #states and #ajax and so on should technically be counted there as well, since they are essentially doing the same as #attached, but merely have been separated into own properties for DX convenience. The pattern for all is the same; they declare a handler to be invoked along with parameters for it. In turn, I think we want to have $response->assets->add(string $plugin_id, array $parameters), whereas $plugin_id would just be 'library', 'css', or 'js' for the most obvious cases.
  2. hook_page_build() #attached is required for "every_page" assets that are not bound to a particular route/block/page, but are needed on all (page) responses.
  3. Let's make sure to not tightly couple Response and AssetBucket, so that AssetBucket and its processing logic/handlers can be swapped out.
  4. The "lookup table" discussed in the asset aggregate compilation phase chapter sounds concerning to me, since it implies that a runtime code to lookup table synchronization mechanism is required, which in turn requires a complex rebuild mechanism. Over the past years, we've eliminated most of these constructs in core. AFAIK, the only remaining one is action.module's action_synchronize() - which, like any other, is ugly as hell. Let's make sure to investigate other options there.
sdboyer’s picture

yeah, it's a mix of summary and new stuff - the AssetBucket is new, as is the separate compile-time pass. a lot of the rest, though, is stuff that's been discussed in one form or another elsewhere. i'm hoping that, by taking this big picture approach, we can take care of the whole problem, better and more or less at once.

Please keep the "arbitrary asset" feature of #attached, which allows it to be used for drupal_add_css(), drupal_add_css(), and drupal_add_library(), but also any kind of "add/build handler" for more sophisticated attached assets; e.g., drupal_add_tabledrag(), etc. #states and #ajax and so on should technically be counted there as well, since they are essentially doing the same as #attached, but merely have been separated into own properties for DX convenience.

this could be tricky. in the discussions i've had with various people about it, retaining the 'arbitrary asset' capabilities was one thing that didn't get heavily emphasized as something that we absolutely need. libraries for sure, but not the arbitrary aspect. really, this whole shift in the page building methodology is about reducing the arbitrary "do anything anytimeness" of the system in favor of more specific, reliable points in time for doing things. but i did figure this would come up. so, good.

my basic concern is that the AssetBucket should really be designed with a slim interface and be focused on handling assets. not anything, but, assets. per your third point, they should of course be loosely coupled at least insofar as we work entirely behind interfaces rather than a specific class. but that interface should really be designed with methods for trafficking in js and css, and extending the class to provide one or another arbitrary other thing is clearly not the way to go.

now, judging from drupal_add_tabledrag(), that oughtn't be too much of a problem: that's really just a helper function that ultimately results in a drupal_add_js() call. it can be refactored to, say, take a AssetBucket as an injected argument, and be pretty much good. #states also ultimately resolves back to #attached, and as such should be able to be translated into a more direct js-adding that AssetBucket can understand. basically, so long as it ultimately resolves back to a drupal_add_{js,css}() call, we're fine. the only question is how many methods we want to put on the bucket, vs. leave as separate helper functions.

hook_page_build() #attached is required for "every_page" assets that are not bound to a particular route/block/page, but are needed on all (page) responses.

not sure what point to take from this, but hook_page_build(), like hook_page_alter(), are destined for /dev/null. they're to be replaced by the text/html controller, displays, and the event listeners that look for the out-of-band info in the Response that're being discussed here.

Let's make sure to not tightly couple Response and AssetBucket, so that AssetBucket and its processing logic/handlers can be swapped out.

as i said above, i'll definitely put these behind an interface to start with. i agree that we should keep an eye on how much more flexibility we'll need as we go, but the overall plan at this level is to make these general-purpose enough that they don't need to be swapped out.

The "lookup table" discussed in the asset aggregate compilation phase chapter sounds concerning to me, since it implies that a runtime code to lookup table synchronization mechanism is required, which in turn requires a complex rebuild mechanism. Over the past years, we've eliminated most of these constructs in core. AFAIK, the only remaining one is action.module's action_synchronize() - which, like any other, is ugly as hell. Let's make sure to investigate other options there.

think of it more like rebuilding the router. it's really a lot more analogous to that.

there's no question that it'll be complicated. but the fact is we'll never be great at building smart bundles unless we're willing to have a separate pass that is able to introspect the entire system. you just can't be that smart if you only have insight onto what the current request has.

tim.plunkett’s picture

Quick note that might seem trivial: I think based on naming patterns from Symfony, that AssetBag might be a good name instead of AssetBucket.

No real concrete input from me, other than THANK YOU @sdboyer for starting this.

sdboyer’s picture

@tim.plunkett oh, i like AssetBag, too. better than bucket, i think. i was really just trying to pick something that indicated the partially-ordered-ness of the assets contained therein, thereby setting it apart from what Assetic's asset managing classes do.

sdboyer’s picture

Issue summary: View changes

Missing ]

sdboyer’s picture

ok, updated the issue summary to use AssetBag instead of AssetBucket. @tim.plunkett++

sun’s picture

re: #5:

1) Perhaps we need to make a differentiation between "low-level assets" and "high-level asset plugin APIs"? It might be OK to limit the low-level asset types, but speaking of, I'd then possibly recommend to add "meta tags" to that low-level list, since it seems we're generally talking about aggregating non-output-related HTML elements as "assets" from sub-request responses to a parent page response.

FWIW, drupal_add_* yields a nice overview of page-level addition helper functions in core, of which most can be used with #attached. Only drupal_process_states() and ajax_pre_render_element() there. I'm using that facility in a lot of contributed and custom modules to conditionally invoke similar functions (and yes, most of them boil down to pure CSS/JS/meta after processing).

Also note that common usage of the term "asset" usually encompasses "file assets" (as in: images, videos, media, etc), too. We're limiting its meaning to CSS, JS, and (potentially) meta tags. (Just noting.)

Since this information makes no sense for the outside world and only serves for intra-Drupal communication, I wonder whether it even makes sense to strip it down to the abstract/generic low-level components, or whether we couldn't just as well pass along the raw parameters for the high-level APIs as we need them. In the end, only Drupal will be able to consume the data (either directly or via HTTP response headers).

2) I'm certainly aware of the called out fate of hook_page_build() and hook_page_alter(), but I do not think it matters for this issue and discussion, because the point I was trying to make is that we need a facility for modules to add assets to all pages. That's important for frontend performance and is a very common use-case in core and contrib, so it should not be an after-thought.

3) k.

4) I'm not sure whether the design you foresee will allow it, but frankly, I'd rather recommend to leave that "asset tracking + lookup" facility out of the (initial) change proposal, as it seems to be a separate proposal of its own for how to improve the CSS/JS aggregation systems. I think there are 2-4 issues that are independently focusing on that topic, and meanwhile, I'd personally suggest to approach a (pure) registry-based solution first; i.e., requiring to explicitly register what needs to be loaded where and remove support for loading anything dynamically on-the-fly — that way, site assets can be truly bundled for optimal performance, because the system has knowledge, and there's no need for manual state tracking and synchronization. But as mentioned, that's a much larger discussion, and one that's being held in various other issues already. ;)

sdboyer’s picture

1)

Perhaps we need to make a differentiation between "low-level assets" and "high-level asset plugin APIs"? It might be OK to limit the low-level asset types, but speaking of, I'd then possibly recommend to add "meta tags" to that low-level list, since it seems we're generally talking about aggregating non-output-related HTML elements as "assets" from sub-request responses to a parent page response.

yep, exactly the sort of differentiation i had in mind. and in that sense, and i'd agree that meta tags are another appropriate thing to add to the list.

that api.d.o list is handy, thanks - hadn't thought to do that.

Also note that common usage of the term "asset" usually encompasses "file assets" (as in: images, videos, media, etc), too. We're limiting its meaning to CSS, JS, and (potentially) meta tags. (Just noting.)

it's very worth noting. i'd done a fair bit of thinking about this where it did encompass files as well, but was convinced (finally and entirely by Snugug) that we didn't need to think about it here. that doesn't mean our docs don't need to make the distinction clear, though.

2) oh, completely agreed. i think you may have misunderstood in the proposal: that sort of capability is what i was referring to in the discussion of "global-ish" assets; they're to have an entire series of listeners dedicated to working with them. in fact, i've been using admin_menu as my canonical example in discussing the need for it.

4) i agree so much, i said it in the proposal :)

Fortunately, we can deal with the bundling/aggregating question *entirely* separately from this base issue. To expedite this patch, we should skip aggregation entirely, and address it later as a performance improvement. That means pushing both the request-time logic for doing the lookups and the compile step for generating the bundles into a followup.

we can build the facility that allows integration of information generated during a compile-time pass, and worry about how we actually do it later. the two are very cleanly separated. we wouldn't be able to work on it right now anyway, as it requires pulling stuff from the routing system in a way that's not quite ready yet.

Damien Tournoud’s picture

I'm having a really hard time figuring out what's new in this proposal. All-in-all, it seems to be a description of the architecture we have since Drupal 7, but with other names.

The canonical way of managing assets since Drupal 7 is #attached, which works precisely the way described here:

  • "Producers" attach assets to different parts of the page (hear: to the main Response and to sub-Responses)
  • The assets are merged together, then aggregated and rendered with the html display; or they are transmitted to the client via an AJAX command

We still have drupal_add_* in Drupal 7, but purely as legacy, deprecated mechanisms. (Technically, we are using those globals briefly during the page generation process, but that's an internal detail)

I'm fine with the refactoring as described, but let's not pretend we are creating anything new or more flexible.

Damien Tournoud’s picture

Also, I agree with @beejeebus that this is purely a pipe dream:

And since it's relatively easy to mock up a Request object in order to do so, it means we just need to load all routes, mock up a Request, and we can introspect all of the Responses produced across the entire system. Bam: full insight into real patterns of asset usage, systemwide.

If we want to make assets route-based, they should be attached to the route. But of course, that means that the whole sub-request architecture (ie. Blocks), completely falls apart.

So it is *not* realistic to think that this will *ever* be remotely true. I suggest removing the whole discussion from the issue summary.

sdboyer’s picture

a lot of the shifts here are lateral. but not all.

really, the most important thing you've played down is that we're getting rid of the drupal_add* mechanisms:

We still have drupal_add_* in Drupal 7, but purely as legacy, deprecated mechanisms. (Technically, we are using those globals briefly during the page generation process, but that's an internal detail)

removing them is not a minor thing. and, deprecated? they're not marked as such, nor are they treated that way in D7 or D8 core, or contrib. their presence undermines stable page structure and cacheability. getting rid of them grants us certainty about what's in the page...well, if you buy into the idea of declarative conditionals. and you don't. that's fine, i already addressed the point in #3; if that doesn't do anything for you, then ok, we'll just have to see how it plays out. i don't intend to remove it from the issue summary, unless it's to shift it into a follow-up issue, though.

all that said, this is useful food for thought. it's pretty late and i'm too involved in specific bits on this patch to see the big picture right now, but maybe we are just better off sticking with renderable arrays all the way up to the top, and just cutting out the page alter. seems like a good fallback, anyway.

i don't quite understand this, though:

If we want to make assets route-based, they should be attached to the route. But of course, that means that the whole sub-request architecture (ie. Blocks), completely falls apart.

the proposed architecture does effectively allow for assets to be route-based, though still loosely coupled (as in, not directly attached to the route). though i fail to see how attaching assets to routes would make subrequests fall apart?

Wim Leers’s picture

#13: I think Damien Tournoud meant that if assets really are attached to routes (the top-level one, i.e. current_path()), that it would by definition be impossible to attach assets to blocks/subrequests, since those are not the top-level route.


Regarding the proposal:

  • Overall: thanks for synthesizing so much information!
  • Overall: it's still pretty complex. However, I'm sure that this at least partially due to the fact that I'm not deeply familiar enough with the D8 architecture (as it currently is but especially as to how it is planned to evolve). In any case I apologize in advance if some of my remarks below don't make sense.
  • Why do we still need to allow for individual CSS/JS files to be added? Why can't we make libraries the smallest unit?
  • Devil's advocate: how is this going to play well with an ESI set-up, where a CDN is coming back for blocks included through ESI? I imagine that it will be necessary/should be possible for a contrib ESI module to prevent block-level (not global-ish level) assets to *not* be referenced from the HTML, but to be loaded dynamically through JS, e.g. by including a <script> tag in an ESI'd block's HTML that includes Drupal AJAX commands to load the CSS/JS. Is this how you want it to work? Should I shut up because this is irrelevant at this point in time?
  • we can produce that canonical list of assets on a route-by-route basis

    This makes me giddy :D The possibilities to optimize the way CSS & JS are bundled…! I agree with sun & sdboyer though that this should not be addressed in this issue.

  • Minor note: global(ish) JS may be important, but don't forget about global(ish) JS settings. I'm very glad we're moving away from drupal_add_js(). Adding global JS settings has always been ugly (often requiring either static variables to ensure it's only added once, or relying on the built-in server-side merging of settings) — #attached didn't solve this. So, while implementing all of this, please keep this in the back of your head and maybe you'll find a way of dealing with this more elegantly.
  • I plan to extend BlockInterface to include two new methods: respond() and getAssets(). The latter will return an AssetBag, and the former one of our custom Response objects with that AssetBag included.

    If I understand this correctly, this assumes that a Block plug-in is all-knowing about the assets that will be used. Will that really be true though? What if a Block is relying on something else to generate content and that something else is using #attached, does that now mean that getAssets() will also have to do #attached processing?

catch’s picture

Devil's advocate: how is this going to play well with an ESI set-up, where a CDN is coming back for blocks included through ESI? I imagine that it will be necessary/should be possible for a contrib ESI module to prevent block-level (not global-ish level) assets to *not* be referenced from the HTML, but to be loaded dynamically through JS, e.g. by including a

tag in an ESI'd block's HTML that includes Drupal AJAX commands to load the CSS/JS. Is this how you want it to work? Should I shut up because this is irrelevant at this point in time?
This is something that got discussed a lot at DrupalCon Munich and again at BADCamp. There are two ideas, not really mutually exclusive: 1. Each block as part of the response object returns its assets. When the block is rendered by an ESI request, we include some JSON with the necessary js to load, that then gets picked up by a scriptloader, which adds the tags to the page dynamically. This makes ESI dependent on that script loader though. 2. Each block has a method that returns the assets. If it includes something like a form with #attached, then it has to at least built the form and pull the assets out with drupal_render_collect_attached() so they're covered by the method. This method then gets called on every block during the 'main' page rquest so that assets can be figured out globally similar to now, but they can be actually rendered by ESI in a separate request still. sdboyer's hoping that we can figure this out per-display rather than per-page to save some of the extra processing this approach would require. Neither of these are perfect but were the best that several people were able to come up with during those discussions.
Damien Tournoud’s picture

1. Each block as part of the response object returns its assets.

As I mentioned previously, this is hard. Many not because of the JS, but because of the CSS. All our JS files support being loaded twice, as long as they are loaded in proper dependency order. The CSS files, on the other hands, don't support being loaded twice, or you break the CSS stack. I don't know if we can solve the CSS issue without a JS solution, which is likely not going to be fabulous in terms of page rendering performance.

2. Each block has a method that returns the assets.

This is what I mentioned in #12 as not being realistic. In the general case, a block can *only* know its assets by fully rendering itself in our recursive rendering model.

Think about this simple example:

 - Page
    - View
       - Node
          - Field
              - Google Maps

The "View" block ultimately depends on the Google Maps assets, but it cannot know that until it generated the View query, displayed a particular node in the proper view mode, that calls a field formatter that adds the assets.

To make this work, we need to mandate every part of the page to know about every possible asset it can possibly need. I do not think that is realistic.

Wim Leers’s picture

#15: Thanks for the info! Approach 1 sounds feasible, approach 2 doesn't sound feasible, for the reasons outlined in #16.2.

catch’s picture

@Damien. Yeah #1 requires the script loader to handle CSS as well, msonnabaum brought up yepnope as one that does. It's definitely hard and there's going to be trade-offs.

#2 yes you'll need to build a render array recursively for every block, which is what I said. If it turns out possible to do this per display as opposed to per path (with constraints that you then don't get to have per-node css etc) then that might be acceptable. Doing it on every request just completely undoes the point of having *si support in the first place which is why I've spent a decent amount of time nagging people to actually tackle this issue. 'addressable blocks' is only one part of getting this all working in a meaningful way.

Wim Leers’s picture

sdboyer’s picture

Everything laid out in #16 is acknowledged in the issue summary; catch pretty much covered my responses there. in the case of an in-process subrequest, the deduping is handled by the AssetBag; in the case of *si, it has to be handled by a scriptloader.

the requirement to fully render oneself in order to know one's assets is one of those problems with our rendering architecture - and one of those reasons we're trying to change that architecture with patches like these. there will be cases where that's what needs to happen in order to report assets, but those aren't the only cases. and the only thing for which that actually makes a difference is if we're trying to achieve the compile-time pass, when all we really care about is the asset reporting. i don't really see what's unrealistic about that.

i'm gonna try to put up a patch later tonight that lays out a solid starting point/basic architecture, but i've come to the conclusion that i need to adjust my original assessment of not using Assetic at all until the out-of-scope-for-this-issue compile step. rather, i think a better approach is to represent assets as classed objects, which a) lets us be a little smarter about them than just having an $options array, b) lets us more clearly delineate between a fully expressive stack of assets vs. a "compiled" representation that's the info we actually need at runtime, c) helps distinguish between an asset that *must* be used on a given page vs. an asset that may optionally be present (tying in to the whole conditional discussion, of course).

so, if they're gonna be classes, might as well repurpose what Assetic has there already to make it easier to apply Assetic's logic once the time comes. there's nothing wrong with the base classes/interfaces Assetic provides - e.g., AssetInterface and AssetBase - so we may as well extend from there and make things simpler at runtime.

now, some answers to Wim in #14:

Why do we still need to allow for individual CSS/JS files to be added? Why can't we make libraries the smallest unit?

i don't really know, but my immediate thought is that it's a bit of a DX wtf that people wouldn't just be able to declare the CSS/JS that go with their blocks. the extra step of making a library for them doesn't make sense.

I imagine that it will be necessary/should be possible for a contrib ESI module to prevent block-level (not global-ish level) assets to *not* be referenced from the HTML, but to be loaded dynamically through JS, e.g. by including a

tag in an ESI'd block's HTML that includes Drupal AJAX commands to load the CSS/JS. Is this how you want it to work? Should I shut up because this is irrelevant at this point in time?
somewhat dealt with before, but yes: blocks *report* their assets in their Response. what we actually *do* with that report is an entirely different question, and is managed by subscribers registered into the container that can be swapped out as needed. there's still a fair bit to be figured out about how we do all the post-controller massaging of responses.
Minor note: global(ish) JS may be important, but don't forget about global(ish) JS settings.
for sure, not forgetting about them. in the code i have that i currently dislike, they're passed up the stack in an AssetBag in the same way as everything else. switching to classes to represent different asset types will mean a bit of thinking for JS settings, but i don't see it really getting any more difficult to handle them. might even kinda cheat and just leave them as directly part of the bag.
sdboyer’s picture

OK, initial patch. this patch is NOT functional, and does NOT include tests, but i think it's enough to advance the conversation.

as i said in #20, i very unhappy with AssetBag in this patch. as it's currently written, it's essentially trying to incorporate all the behavior we have in drupal_add_css() and drupal_add_js() into its various methods, and managing all that ends up making for very redundant code that could be a lot better documented. more importantly, it conflates two responsibilities together into the one class: managing the internal characteristics of the assets (file vs. external, whether or not to include in bundling a la preprocess) vs. the ordering, scoping, etc. of the assets relative to one another.

note that AssetBag is also really incomplete. the thing i find most troublingly incomplete is how it handles recursion: the AssetBag::addAssetBag() method, which simply stores the provided asset bag in a separate array. the biggest problem there is that it's not recording the order in which the bags are added relative to the normal assets, which i think is necessary for resolving correct ordering when flattening down all the bags.

so in a subsequent patch, which i hope to have up in the next couple days, i'll solve the separation of concerns issue by creating a class to encompass assets themselves, probably by utilizing Assetic\Asset\BaseAsset as the baseline. in order to make the recursive resolution work better, i'll probably follow the pattern set with AssetCollections and have AssetBagInterface inherit from Assetic\Asset\AssetInterface - that is, assets and bags will all implement the same base interface.

and, i feel i must mention that always working with an Asset class to represent each asset will probably make it easier to deal with the conditional asset problem later. i think it'll make it easier to take an approach where assets are always listed, but with conditional logic attached to them that is evaluated elsewhere, rather than expecting, say, global-ish hook responders to inspect global context and *only* return the assets that are needed in the *current* environment. a declarative minisyntax like that is still gonna be tricky to do, but this is the sort of way we'd need to do it for that to ever work.

OK, so, that's what i DON'T like about this patch. here's what's in it that i do like, ish:

  • A Drupal\Core\PartialResponse class, which is the basis for the Response class described in the issue summary. Significant changes to this, though - more on that below.
  • An HtmlViewSubscriber which is designed to work with the PartialResponse.

first, the PartialResponse: after nearly staring a hole at my screen while looking at basic request handing in Symfony, i came to the conclusion that we'd be better off NOT subclassing their Response because of how this logic works:

    private function handleRaw(Request $request, $type = self::MASTER_REQUEST)
    {
        // ...
        // (earlier irrelevant bits removed for brevity)
        // ...

        // call controller
        $response = call_user_func_array($controller, $arguments);

        // view
        if (!$response instanceof Response) { // <-- THIS is why
            $event = new GetResponseForControllerResultEvent($this, $request, $type, $response);
            $this->dispatcher->dispatch(KernelEvents::VIEW, $event);

            if ($event->hasResponse()) {
                $response = $event->getResponse();
            }

            if (!$response instanceof Response) {
                $msg = sprintf('The controller must return a response (%s given).', $this->varToString($response));

                // the user may have forgotten to return something
                if (null === $response) {
                    $msg .= ' Did you forget to add a return statement somewhere in your controller?';
                }
                throw new \LogicException($msg);
            }
        }

        return $this->filterResponse($response, $request, $type);
    }

if the controller returns anything that's an instanceof Symfony\Component\HttpFoundation\Response, then we skip the whole View event. originally i was thinking this would be fine, as we could do what we needed to in the RESPONSE event, which is what's triggered in $this->filterResponse(). and that's true. but, on further reflection, the task at hand - doing final page composition by taking an HTML string and wrapping it with, basically, html.tpl.php, is really something that is the conceptual domain of the View. practically, this is important because it will help delineate between different types of Requests that Drupal may serve, which means less "if" statements in our subscribers overall. and of course, less wtf for people coming in from the outside.

so, PartialResponse does not subclass Symfony's Response as i originally suggested, so that we DO run through that event. the methods currently on that class are geared towards setting Assets, main content, and data (such as page title and meta tags) that are typically handled in html.tpl.php. i'm perfectly happy to add more.

we do have to eventually arrive at a full Symfony Response, and that's where the HtmlViewSubscriber comes in. it precedes the current ViewSubscriber we have in priority (not sure how much we'll want that thing in the long run, anyway), and is responsible for taking a PartialResponse and turning it into a full Response. it does what's described in the issue summary - getting assets from the theme, calling out with a hook to harvest additional assets, marshalling them all together, passing them along to an optional bundling step, then pulling all that together and passing through our html template file. all VERY pseudocodey, which can become less the case once progress is made on the asset classes, but it's a good basis for discussion.

sdboyer’s picture

wow. assets are complicated.

i've decided to split this issue, and make this one specifically focused on *just* the Response stuff, independent of the assets. for the assets, i'm hijacking #1762204: Introduce Assetic compatibility layer for core's internal handling of assets, since that's basically what it's about anyway. i'll move the relevant discussion over there presently.

i'll also have a more focused PartialResponse patch later today, hopefully including some workarounds to get it to shoehorn nicely into html.tpl.php. after that, not much more will need to be done in THIS issue - the rest can be followups.

once that patch is posted, though, i desperately need to switch gears back to doing work on controllers, so i'll be putting up some posts on g.d.o etc. looking for people to come help with this :)

sdboyer’s picture

Status: Active » Needs review
FileSize
7.85 KB

retitling appropriately, based on the modified approach.

the attached patch is just stupid-simple, but it's a step, anyway. it introduces the PartialResponse class with only a single focus - holding the string content of the response. it'll need to be extended, in followup issues, with methods for containing the other variables used in populating html.tpl.php. but for now. i'm updating the issue summary to that effect.

i've been chasing my tail a bit on a good way to write a test for just this functionality, but rather than wait any longer on it, i figured i'd just get it up. to that end, i'm not actually sure if the approach will work as expected - calling theme('html') with a $page variable that's a render array of type #markup instead of #page - but it seemed like a good starting point.

sdboyer’s picture

Title: Create a new class extending Symfony's Response that manages out-of-band assets » Create a PartialResponse class to encapsulate html-level data

cept i didn't actually retitle - whoops.

sdboyer’s picture

Issue summary: View changes

change AssetBucket to AssetBag

Crell’s picture

This seems like a move in the right direction. I think drupal_render_page() is better than theme('html'); se the existing View subscriber.

Can we try using this somewhere, like say shoving it into the existing HtmlPageController as a wrapper, just to make sure it all works?

sdboyer’s picture

FileSize
10.81 KB

@Crell - err...the existing View subscriber, and drupal_render_page(), is exactly what we DO NOT want. the whole idea of the PartialResponse is that it contains the finalized html string output of some pagebuilding (or blockbuilding) logic.

there are now tests that demonstrate it is capable of just spitting out the html template, sans all the additional stuff that gets added at the page.tpl.php level. the only way we could make it work with the existing HtmlPageController is if we were to move the old pagebuilding logic, drupal_render_page(), directly into that controller, then encapsulate the result into the PartialResponse. imo, that's a rearrangement of deck chairs that tells us nothing more about the functionality of the patch than the tests do in this latest version that i've put up.

there are a couple of rough edges in the patch, one of which i'd like a bit of help with, but the others i think should be addressed in the respective followups. the one i'd like help with are the @todos related to the new module_handler service - i'd like to inject it so that i don't use the procedural module_implements() and drupal_alter(), but the service seems to be absent when the subscriber gets instantiated by the sandboxed instance on the other side of a WebTestCase web request - nothing gets passed in, and there are errors. i've included the code, albeit commented out, that SHOULD work. this might be a wider problem with the container in web test cases, in which case i'd prefer this go in and we uncomment that stuff once it gets figured out.

the other stuff that's rough, but does need to stay in for now, is the fact that we're invoking hooks at all - specifically, the page build & alter hooks. the ONLY reason they're there is so that we can easily retrieve the $page_bottom and $page_top variables for injection into the template. my intention is that we get rid of those variables entirely in a later patch (both of those vars should be pushed down into block-level handling). so they'll be pulled out as part of work in #1812720: Implement the new panels-ish controller [it's a good purple].

these caveats aside, this thing gives us the foundation to take out the other stuff we need to. so...tally ho.

sdboyer’s picture

oh, and - suggestions welcome on a better place to put that test, and a better group for it.

katbailey’s picture

FileSize
3.52 KB
10.57 KB

I was looking into why injecting the module handler was problematic, but everything works fine for me locally when I uncomment that part of the patch. Let's see what testbot thinks...

sdboyer’s picture

@katbailey - SWEET. thanks. not sure why it wasn't working on my local - i may have been a bit out of date.

ok so, this is now ready for serious reviews and potential RTBCing. have it at, peoples :)

aspilicious’s picture

Can we have a quick reroll for this?

+++ b/core/modules/system/tests/modules/html_test/html_test.moduleundefined
@@ -0,0 +1,4 @@
\ No newline at end of file

Needs a newline

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -0,0 +1,101 @@
+    // Modules can add elements to $page as needed in hook_page_build().
+    foreach ($this->moduleHandler->getImplementations('page_build') as $module) {
+      $function = $module . '_page_build';
+      $function($page);

Why is this not just invokeAll()?

+++ b/core/lib/Drupal/Core/PartialResponse.php
@@ -0,0 +1,55 @@
+/**
+ * Response object that contains variables for injection into the html template.
+ *
+ * @todo move these methods into an interface.
+ * @todo add method replacements for *all* data sourced by html.tpl.php
+ */
+class PartialResponse {

I know we don't have all of the out of band stuff in place yet, but can we get something hinting in that direction on this object? It's hard to see how it all fits together yet.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.php
@@ -0,0 +1,47 @@
+  /**
+   * Tests that a PartialResponse object is rendered correctly.
+   */
+  public function testPartialResponseRendering() {
+    // First, test it with a direct request.
+    $this->drupalGet('html_test');
+    $this->assertRaw('hello world');
+
+    // Now, mock up a kernel and test it that way, too.
+    $kernel = $this->container->get('http_kernel');
+    $request = Request::create('/foo');
+    $response = new PartialResponse('hello world');
+    $subscriber = new HtmlViewSubscriber($this->container->get('module_handler'));
+    $event = new GetResponseForControllerResultEvent($kernel, $request, HttpKernel::MASTER_REQUEST, $response);
+    $subscriber->onPartialHtmlResponse($event);
+    $this->assertTrue(preg_match('/hello world/', $event->getResponse()->getContent()), 'Expected raw output found within HTML response.');

These should be 2 separate methods.

Also, this needs to test not just the presence of hello world, but that there is an HTML page wrapped around it but only ONE HTML page. (I ran into that inception problem a lot with the early kernel patches.)

sdboyer’s picture

I know we don't have all of the out of band stuff in place yet, but can we get something hinting in that direction on this object? It's hard to see how it all fits together yet.

fair enough - i'm happy to add methods for all the data i anticipate the PartialResponse carrying, then filling them in as we go. will definitely make things clearer for people who are not me :)

These should be 2 separate methods.

bleeeeeeeeeeeh extra install time for a second method. maybe i'll see if the latter can run as a unit test...it'll probably fuck shit up horribly, since calling theme('html') eats so many global vars.

Also, this needs to test not just the presence of hello world, but that there is an HTML page wrapped around it but only ONE HTML page.

i spent a while looking at the html output generated for these and did visually verify that, but yeah, never wrote in any code for it. unless you have a quick & better suggestion for how to do it, i'll prolly just grep the output to ensure that there are the correct number of html/head/body tags.

Crell’s picture

This is the chx-approved way to check against Drupalception in a WebTest:

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

sdboyer’s picture

FileSize
12.46 KB
8.11 KB

we don't use invokeAll() because that doesn't allow passing by reference, which hook_page_build() requires (yeah, it's totally not like an alter at all...).

i've added at least a few illustrative method implementations on PartialResponse, split the tests, and added the newline. what else we got?

Fabianx’s picture

#34: So what is the different than between hook_page_alter and hook_page_build?

Just the name? Just curious.

effulgentsia’s picture

So what is the different than between hook_page_alter and hook_page_build?

These will likely go away eventually with #1812720: Implement the new panels-ish controller [it's a good purple], but currently, hook_page_build() is for adding things to $page (e.g., block.module adds blocks, toolbar.module adds the toolbar), and hook_page_alter() is for letting modules alter what has been added.

sdboyer’s picture

indeed, build is for adding, alter is for changing - i was just commenting that there's little functional difference between them, since build passes the page render array by reference.

any reviews? :P

webchick’s picture

Issue tags: +Blocks-Layouts

Tagging.

Crell’s picture

+++ b/core/lib/Drupal/Core/PartialResponse.php
@@ -0,0 +1,128 @@
+  /**
+   * Adds another meta tag, to be output in HTML head.
+   *
+   * @param $tag
+   */
+  public function addMetaTag($tag) {
+    // @todo implement this
+  }
+
+  /**
+   * Gets all meta tags for output.
+   */
+  public function getMetaTags() {
+    // @todo implement this

I wonder if we shouldn't be using a Bag for this. Just a side note.

In concept this seems sensible. I'd like to see it combined with one of its sibling threads to see what the total effect is. It's hard to really get a grasp for these issues in isolation.

catch’s picture

sdboyer’s picture

FileSize
12.33 KB
965 bytes

#39 - yep, i think a Bag would work well there.

rerolled patch; removed '#type' and '#markup' keys from the html render array, as they were superfluous.

i'm not seeing anything in these reviews that indicates there's a problem with RTBCing...somebody want to take the leap on that?

catch’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,95 @@
+  public function onPartialHtmlResponse(GetResponseForControllerResultEvent $event) {
+    // Only act if we have a specialized Drupal response to work with.
+    if (!($response = $event->getControllerResult()) instanceof PartialResponse) {
+      return;
+    }
+
+    // @todo this is really lazy, but the easiest way to get $page_{top,bottom}. this can be removed once the responsibility is shifted into displays/controllers
+    $page = element_info('page');
+    foreach ($this->moduleHandler->getImplementations('page_build') as $module) {
+      $function = $module . '_page_build';
+      $function($page);
+    }

So this is going to run hook_page_build() and hook_page_alter() for every single partial response object? The result will be exactly the same each time won't they? Those hooks are proper 'global' hooks that can reasonably expect to be run once per request so not sure about this at all.

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,128 @@
+  /**
+   * Sets the AssetBag for this PartialResponse.
+   *
+   * The AssetBag has all of its own logic for sorting out assets and deciding
+   * where they should go; the PartialResponse is only response for carrying it
+   * around.
+   *
+   * @todo https://drupal.org/node/1762204
+   */
+  public function setAssetBag() {
+

I'd just drop this until that patch is in, same with addMetaTag().

sdboyer’s picture

Status: Needs work » Needs review
FileSize
11.7 KB
2.47 KB

So this is going to run hook_page_build() and hook_page_alter() for every single partial response object? The result will be exactly the same each time won't they? Those hooks are proper 'global' hooks that can reasonably expect to be run once per request so not sure about this at all.

ah, hadn't thought of that. at the moment the PartialResponse isn't in use anywhere, and though we certainly intend to use it for blocks, the thing that needs it first is the page-level controller - for which that's fine, as it only would be run once per page. i've been imagining that we would refactor those things out as we fix up the html templates and preprocess functions such that it would be applicable for blocks as well.

however, there's an easier fix that'll help us accommodate blocks in the short-to-medium term - in the new patch, i've wrapped that in a conditional so that we only invoke page build/alter if we're on the main request.

I'd just drop this until that patch is in, same with addMetaTag().

agreed. i added it at the request made in #31.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

This is looking pretty ready to roll to me. Let's get this in.

Eclipse

lunaris’s picture

Agreed; I think the next key developments will be in issues like #1762204: Introduce Assetic compatibility layer for core's internal handling of assets -- let's commit this!

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

General feedback: in the code comments: "Response", "Event", etc. are capitalized because they refer to specific object types (i.e. instances of specific classes), but I don't think this a common practice in core?

I understand that you want to move forward, but there's a lot of messy docs (plus more nitpicks), and unfortunately also quite a few things that are not very clear to me, and probably to most of us who don't know the entire new Request/Response architecture well. Some things should probably be explained better

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ * Definition of Drupal\Core\EventSubscriber\HtmlViewSubscriber.

Contains \Drupal…

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ * Subscriber for for VIEW HTTP events that finishes a Drupal HTML response.

Why is "view" capitalized?

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ * with assets and wrapping the in the skeletal HTML, primarily the <head> tag

"the in the"?
Possibly "them in the".

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ *
+ */
+class HtmlViewSubscriber implements EventSubscriberInterface {

Extraneous blank line.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+   * @return \Symfony\Component\HttpFoundation\Response;

Trailing semi-colon.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+    if ($event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) {
+      foreach ($this->moduleHandler->getImplementations('page_build') as $module) {
+        $function = $module . '_page_build';
+        $function($page);
+      }
+      $this->moduleHandler->alter('page', $page);

I think what this is doing is "If this is not a subrequest, but the master request, then invoke all hook_page_build hooks and the hook_page_alter hook.

This could use a comment to that effect; also clarifying why it is that you only want this to run for the master request.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+    // Stop propagation, as the ViewSubscriber will handle this poorly.
+    $event->stopPropagation();

Maybe this is because I don't know the new Request/Response architecture well enough, but this sounds very vague to me.
i.e.: I don't really know what this means.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+    return $events;
+  }
+}

Shouldn't there be a newline before the brace closing the class, like there is a newline after opening the class?

+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.phpundefined
@@ -152,7 +152,7 @@ public function onHtml(GetResponseForControllerResultEvent $event) {
+    $events[KernelEvents::VIEW][] = array('onView', 30);

So the weight is being changes to be lower than the onPartialHtmlResponse event. Shouldn't this be documented?

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+ * Definition of Drupal\Core\PartialResponse.

Contains \Drupal…

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+ * Response object that contains variables for injection into the html template.

s/html/HTML/

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+ * @todo move these methods into an interface.

Can't this be done in this issue/patch already? If not; what is this blocked on?

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+   * Gets the main content of this Response.

PartialResponse?

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+   * including the title in HTML head.

?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.phpundefined
@@ -0,0 +1,56 @@
+ * Contains Drupal\system\Tests\Common\HtmlViewSubscriberTest.

Contains \Drupal…

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.phpundefined
@@ -0,0 +1,56 @@
+      'description' => 'Tests that PartialResponse data is properly rendered into the html template.',

s/html/HTML/?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.phpundefined
@@ -0,0 +1,56 @@
+   * Tests that a PartialResponse object is rendered correctly by issuing a web
+   * request against it.

s/web request/HTTP request/?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.phpundefined
@@ -0,0 +1,56 @@
+    // Check that our basic body text is there

Missing trailing period.

+++ b/core/modules/system/tests/modules/html_test/lib/Drupal/html_test/TestController.phpundefined
@@ -0,0 +1,18 @@
+ * Contains Drupal\html_test\TestControllers.

Contains \Drupal…

sdboyer’s picture

ach, not sure why i missed this for five days...sorry!

i do want to move forward, but i have no problem with reviews that point out confusing things if they're preventing us from doing so :) thanks!

re: capitalizing classes - this may well not be a common practice, i'm honestly not sure. i mostly did it as it's the same way that i communicate about such things in IRC - i utilize the capitalization in order to indicate that i'm not talking about a loose concept, but a clearly defined segment of code. if i'm not consistent about it, then that's definitely a problem, but it just seems like good sense to me to do so. if it's actually frowned on, though, i'll remove it...

and, as a general note, i am MORE than happy to run anyone who is curious about this through the thought process over IRC or voice. i do what i can with the writeups, but there's a lot that's changing here and a lot of unfamiliar concepts, so i'm happy to do that if it's needed.

i'll post a new patch soon. responding in the meantime - if i skip it, assume you're right and i'm making the fix.

+ * Subscriber for for VIEW HTTP events that finishes a Drupal HTML response.

Why is "view" capitalized?

because it refers to the KernelEvents::VIEW class constant which identifies the type of event being dispatched. that's me being inconsistent - i should have included the full KernelEvents::View.

+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.phpundefined
@@ -152,7 +152,7 @@ public function onHtml(GetResponseForControllerResultEvent $event) {
+ $events[KernelEvents::VIEW][] = array('onView', 30);

So the weight is being changes to be lower than the onPartialHtmlResponse event. Shouldn't this be documented?

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ // Stop propagation, as the ViewSubscriber will handle this poorly.
+ $event->stopPropagation();

Maybe this is because I don't know the new Request/Response architecture well enough, but this sounds very vague to me.
i.e.: I don't really know what this means.

i'm not exactly sure where we'd document it? the ViewSubscriber has always been a shim, though - a way of making sure that our existing menu system's page callbacks would get turned into a proper Response objects. the goal has been (so Larry and i have discussed) that this system we're building here supplants that subscriber, for the most part.

it may not supplant it entirely, though - which is why we're doing what we're doing there. the issue is that HtmlViewSubscriber vs. ViewSubscriber are serving two very different possible types of data that the controller might produce: the former operates only on PartialResponse objects, the latter is built for strings. we stop event propagation at the end of HtmlViewSubscriber because it has done the work of producing a Response. i think this is correct architecturally because producing a Response is the explicit purpose of the event (it's called GetResponseForControllerResultEvent); since a Response has been produced, it is logical to conclude the event. it's useful from a practical angle too, since ViewSubscriber is *really* intent on assuming that the result is either a string or a render array.

we could (and, thinking on it now, maybe should) change the ViewSubscriber to opt out if the controller results is something *other* than a string.

+++ b/core/lib/Drupal/Core/PartialResponse.phpundefined
@@ -0,0 +1,92 @@
+ * @todo move these methods into an interface.

Can't this be done in this issue/patch already? If not; what is this blocked on?

it certainly could. but what i should really add there is a ? at the end - i'm not sure having an interface for this is the best idea. Symfony doesn't have an interface for its Response class, and i'm not sure that modifying the PartialResponse - which is really just supposed to be a thin bundle for renderable data - will ever be the appropriate place to modify things. pretty much anything that goes in there i imagine should be able to be mutated somewhere else. and, since my overall mantra here is increasing page stability by reducing redundant/unnecessary flexibility...

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.phpundefined
@@ -0,0 +1,56 @@
+ 'description' => 'Tests that PartialResponse data is properly rendered into the html template.',

s/html/HTML/?

mmm...maybe. that's actually meant to refer to html.tpl.php, or html.twig.html (lol), so it's kinda another case of trying to use capitalization to be precise about what i'm referring to. ...but then failing to go all the way by writing the full thing out. i'll change it.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.phpundefined
@@ -0,0 +1,98 @@
+ if ($event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) {
+ foreach ($this->moduleHandler->getImplementations('page_build') as $module) {
+ $function = $module . '_page_build';
+ $function($page);
+ }
+ $this->moduleHandler->alter('page', $page);

I think what this is doing is "If this is not a subrequest, but the master request, then invoke all hook_page_build hooks and the hook_page_alter hook.

This could use a comment to that effect; also clarifying why it is that you only want this to run for the master request.

yep, that's what it's doing. i'll put a comment in. i didn't include one i think because this is intended to be a VERY temporary shim, until we have a replacement to $page_top and $page_bottom - right now, overlay and toolbar both use the former. they need to be made into blocks and placed in a real region, and we need to get rid of those html.tpl.php variables entirely. though, it being a temporary shim makes it all the more important that it gets a comment :)

in the meantime, though - we only want it for the master request because if it's a subrequest we have a PartialResponse, it (in the current thinking) will *definitely* mean we're rendering a block. that is, not a page - so it makes no sense to invoke the page build process, and we don't need those $page_top or $page_bottom vars. ...yeah, a giant fat hack.

sdboyer’s picture

FileSize
12.43 KB
6.47 KB

updated patch, fixing the issues raised by Wim in #46. I also made the ViewSubscriber bail out if an object comes through as the controller result. That may or may not be quite right...let's see what testbot says :)

i get that this is still somewhat abstract for people, as it's intended to undergird a system that isn't entirely fleshed out yet. but we've got tests, and it works for the case i've designed it for here. i'd like to get this in...

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, responses-1871596-48.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -0,0 +1,98 @@
+    if ($event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) {
+      foreach ($this->moduleHandler->getImplementations('page_build') as $module) {
+        $function = $module . '_page_build';
+        $function($page);
+      }
+      $this->moduleHandler->alter('page', $page);

Would this actually go away with the Twig conversion, since that eliminates most "page level variables" as pre-rendered values in favor of lazy-created Twig properties?

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -0,0 +1,98 @@
+    $event->setResponse(new Response(theme('html', $vars)));
+    // Stop propagation; the ViewSubscriber is expecting a string,
+    $event->stopPropagation();

setResponse() already calls $this->stopPropagation(), so this line is unnecessary.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/PartialResponseTest.php
@@ -0,0 +1,56 @@
+  /**
+   * Tests that a PartialResponse object is rendered correctly by mocking the
+   * environment.
+   */
+  public function testPartialResponseByUnit() {
+    $kernel = $this->container->get('http_kernel');
+    $request = Request::create('/foo');
+    $response = new PartialResponse('hello world');
+    $subscriber = new HtmlViewSubscriber($this->container->get('module_handler'));
+    $event = new GetResponseForControllerResultEvent($kernel, $request, HttpKernel::MASTER_REQUEST, $response);
+    $subscriber->onPartialHtmlResponse($event);
+    $this->assertTrue(preg_match('/hello world/', $event->getResponse()->getContent()), 'Expected raw output found within HTML response.');

This seems like it can/should be in a DrupalUnitTestBase, not WebTest, no?

I think this is close to being able to get in and move forward, as soon as testbot approves.

sdboyer’s picture

FileSize
1.41 KB
11.77 KB

what lets the page build/alter go away is not so much twig as it is moving things that are currently added directly to $page_top and $page_bottom - overlay and toolbar - into blocks, and letting the controller handle it.

it does indeed call stopPropagation() - i must have never looked in its parent class. whoops. removed.

the latter test could be a DrupalUnitTestBase, yes, but the former makes an actual web request, and so needs to be a WebTestBase. we could separate them into two tests, but i thought it better to have them in a single file simply for sanity and grouping purposes.

the patch pulls out the addition to ViewSubscriber in the event that that was the thing that was causing the test failure (i can't really check, as my local is borked atm). it wasn't really necessary, anyway.

sdboyer’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, responses-1871596-52.patch, failed testing.

sdboyer’s picture

i don't know why it's failing to enable a module...anyone have insight?

sdboyer’s picture

Status: Needs work » Needs review
FileSize
12.34 KB
864 bytes

oh. we switched to yaml.

ok, restoring the ViewSubscriber change, and switched the html_test info file over to yaml.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#56 looks good

moshe weitzman’s picture

I think the direction here is fine, but the patch doesn't do enough to merit getting committed at this point in the cycle. I would like to see the asset handling worked out in this patch. That would justify the addition of this code for me. We don't have to convert of core in the same issue, but the tests should use the new asset pipeline.

sdboyer’s picture

@moshe weitzman - if there's general agreement on that point, then we'll move this work into the sandbox and do everything together over there.

sdboyer’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: -Stalking Crell

actually, yknow what - no sense in appealing to a "general consensus" that is difficult to define and would just waste time we don't have. no worries, we'll move this to the sandbox.

postponing accordingly.

catch’s picture

Which sandbox and what is 'everything together'?

nikkubhai’s picture

This sandbox: http://drupal.org/sandbox/eclipsegc/1441840 .

The things mentioned in this post : http://groups.drupal.org/node/287563

catch’s picture

Status: Postponed » Needs work

There is absolutely no reason this needs to be tied into everything about blocks and layouts everywhere. It's a required part of having any HTML-returning stuff doing that in response format otherwise we've just broken #attached anywhere above that level.

sdboyer’s picture

Status: Needs work » Reviewed & tested by the community

@catch - ok, i'm fine with it going in, too. but if you're gonna set it back to something active, it should be RTBC, as that's where podarok put it before :)

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work

I don't think it has to be tied to everything either (contextual blocks, layout, etc,). I do think the asset handling needs to be included. I don't care where the development happens. Reseting back to catch's status. Sorry for the status pong.

sdboyer’s picture

...ok guys, now i'm just confused. @moshe weitzman - why set it back to catch's status? there are currently no outstanding reviews or issues in this thread indicating problems with the patch. i have nothing further to add to it. what should i be working on? if your status change is to reflect the fact that you feel assets need to be included then it's your status change, not catch's. and also, if assets are needed for this to go in, then i'm working on it in the sandbox, as i said in #63. i'm doing it that way because i don't want to work on assets without also working on converting blocks to using them, because that's another arbitrary line-drawing that will just waste more time and give me a headache...and because, i PROMISE you that when the overall assets system is finished, nobody will get it until blocks are converted, anyway. i'm not setting myself up for that, tyvm.

basically, deal with the patch as it is now without adjusting its scope (in which case, please set to RTBC or offer a reason that podarok's RTBC in #57 isn't accurate), or i work on it in the sandbox (and please set it to postponed). i honestly don't care which one we do, i just want this decided so i can write the code.

podarok’s picture

My review was based on summary deep grocking and patch reading line to line
All described in summary very easy converted into patch and my desicion - RTBC
I`ll be appreciate for more deep review if there are any code bugs or suggestions

EclipseGc’s picture

Status: Needs work » Reviewed & tested by the community

Not to get all soap-boxy here, but this issue has conformed pretty closely with what has been preached to us by long time core contributors since we started which boils down to "You don't have to integrate your feature, you just have to provide tests that prove it will work when it comes time to integrate it." If you feel it's not met that standard then, ok that's probably a separate discussion, but getting it into core is one less thing we'll have to consistently rebase for in the sandbox, and I think the fewer of those things we have the better. No?

I'm setting this back to RTBC. If we can't agree with what I wrote in the previous paragraph, that's ok but just set it back to postponed.

Eclipse

moshe weitzman’s picture

@EclipseGC - As you know, those rules have changed since we are past feature freeze. I think the current patch delivers half a solution and thats not appropriate right now. I get that others disagree, so I'll just leave the status as is for now.

EclipseGc’s picture

Moshe,

So you feel this would break Dries' definition of "shippable"? That seems like a reasonable response to me. Sam and I are 100% on board with postponing this and just doing it in the sandbox. So don't feel like we're trying to bully this into core on its own.

Marking it postponed is 100% ok, but I think you and catch need to come to an agreement at this point and just let us know. We're happy to go either way here.

Eclipse

webchick’s picture

I think the main question to answer here (now that we're post feature freeze) is whether this patch pushes core forward on its own, or whether this pushes core into a "halfway" state and requires additional work to push it forward, which may or may not get done. Clean-up phase is not the time to introduce things that require more clean-ups.

EclipseGc’s picture

webchick,

Yup, we're cool with that, just tell us what the consensus is and we're happy to comply. Seriously, no one needs to stress about it.

Eclipse

webchick’s picture

I don't know enough about this patch to answer one way or another. :) it's more a question for you and Sam. If we cut a 8.0 tag tomorrow, would this patch in its current state be a benefit or a detriment? Is the asset stuff required for it to be an benefit?

EclipseGc’s picture

It's not really beneficial either way. It will only be beneficial when we have assets, displays and blocks all interacting together. Until that time it's just an incremental step forward. It shouldn't harm core in any way either, and would provide for more of the architecture we're pursuing to be in core (obviously) but if it were committed and we end up having to play defense keeping it the way we want it so that when the big picture comes together it works the way we need instead of other people trying to tweak this to make it useful, then we should DEFINITELY postpone the issue and just do it in the sandbox.

Still all of that said, the issue here isn't between us and moshe, or us and catch, I think it's really catch and moshe need to get on the same page. We are not really all that vested in the final decision, just let us know what it is.

Eclipse

moshe weitzman’s picture

AFAICT, catch and I are in agreement - Needs Work. See #63

catch’s picture

Status: Reviewed & tested by the community » Needs review

It will only be beneficial when we have assets, displays and blocks

That's not what I understood the intention of this issue to be.

Anything that returns a response that's going to be included in an HTML page needs to be able also include information about stuff that's going to get stuffed into <head>. The Drupal 7 version of this is to return a render array instead of a string, anything including assets via drupal_add_*() is completely broken and should be taken out and shot. If we're going to have anything return responses (msonnabaum just asked in irc why we don't just subclass response and use that everywhere instead of having a 'PartialResponse' which isn't really answered in this issue as far as I can see), then it needs to be able to handle that, otherwise it's broke.

EclipseGc’s picture

Title: Create a PartialResponse class to encapsulate html-level data » Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]
Status: Needs review » Postponed

Ok, well in an effort not to get bogged down in discussing what the final architecture should look like on this (since I think we'll spend a long time arguing about agreeing) I'm just going to postpone it and do the work in the sandbox and we can be more detailed about the plans in irc if necessary and capture that detail in follow up posts to this issue.

Thanks!

Eclipse

EclipseGc’s picture

Crossposted with catch. I think he has some valid points there. I'll see if I can get sam to perhaps elaborate on them.

Eclipse

sdboyer’s picture

@catch - the reasons for not extending Response directly are detailed in #21. i know @msonnabaum was asking about this too, and we'd actually discussed it before - right now, it's just a render object. we may *also* want to let it do some things that responses do, as well - setting an HTTP response code, for example - which would be a reason to leave it with the current naming. but yeah, right now, it's effectively just a render object.

and, yeah, you're right - we could make use out of this even without displays or blocks or any of that. sorry, i should have realized that when @EclipseGc asked me to read over #72 before he posted it.

i expect that getting all of that together can probably be a closer milestone than the displays & controller...but yeah, sandbox is the way to go.

msonnabaum’s picture

I'd feed better if we weren't calling it a response at least for now, because that's not the role it's playing. Renderable* seems sensible to me.

effulgentsia’s picture

I agree with msonnabaum about not calling it a PartialResponse since per #21, it's intentionally not a HttpFoundation\Response object.

we may *also* want to let it do some things that responses do, as well - setting an HTTP response code, for example - which would be a reason to leave it with the current naming.

Maybe I'm wrong, but my hunch is that even if this object grows to include HTTP status codes and cache tags, we're still not going to intend people to call send() on it, which for me becomes the fundamental thing that makes this not a Response. Instead, it's a data object that a KernelEvents::VIEW event needs to turn into a full-fledged Response (on which send() can be called).

Renderable* seems sensible to me.

It's not really anything like a render array either though. It has no recursion support, by design, since the whole point is to eventually have a simple list of JS and CSS assets, not have to gather them by walking the entire render array tree, looking for all the #attached on elements that aren't also #access=FALSE. So I think we're still going to want most of our controllers returning render arrays, have an early running VIEW event subscriber drupal_render() that array into this intermediary object (flattening #attached into simple JS/CSS arrays on this object), and then a later running VIEW event subscriber turn this intermediary object into a full Response (similar to what the HtmlViewSubscriber in this patch does, but more refined).

If my understanding above is correct, then I'd like to suggest HtmlFragment as the name of this class.

I think the direction here is fine, but the patch doesn't do enough to merit getting committed at this point in the cycle. I would like to see the asset handling worked out in this patch.

Per #1939758-1: Figure out a better way to get the page title to a modal dialog, having this class land in core sooner than asset management is worked out could serve the need for a title+content object needed by modal dialogs. For that, we would likely only use the class itself, not the HtmlViewSubscriber, so if there's resistance to that subscriber being committed to core with all its outstanding todos, we could just settle on the name for the PartialResponse/HtmlFragment/OtherSuggestion class and move it into that issue's work.

sdboyer’s picture

@effulgentsia - i'd love to call it HtmlFragment, or something like that. unfortunately, symfony jacked us out of the "fragment" namespace. which...well, i'll paste my convo with @msonnabaum about this:

<msonnabaum> RenderablePageFragment doesn't seem terrible to me
<sdboyer> HtmlViewSubscriber is pretty much entirely the logic you saw in DrunkController::wrapHtml()
<msonnabaum> I'll make a quick comment about this
<sdboyer> except that symfony has land-grabbed the Fragment namespace for describing not a "thing", but a way you get a thing
-*- sdboyer rolls eyes
<msonnabaum> honestly, I'm not concerned about stepping on their names
<sdboyer> ordinarily i wouldn't care too much about conflicting about their names
<msonnabaum> I mean the word fragment does mean something
<msonnabaum> its not like bundle
<sdboyer> except that in this case, we would referencing both senses of fragment in the very same methods
<sdboyer> you would call a (symfony) fragment in order to get a (drupal) fragment
<sdboyer> no bueno
<msonnabaum> ugh
<msonnabaum> ok, nevermind
<sdboyer> yeah, like i said
<sdboyer> i like their original names
<sdboyer> the new ones are STUPID
<msonnabaum> RenderableShit
<sdboyer> i think i even commented on that PR that i liked the names
<msonnabaum> RenderablePartial is not terrible
<sdboyer> i don't know why the fuck they changed them
<msonnabaum> partial just makes me think of template partials
<msonnabaum> but it probably doesn't matter
<sdboyer> honestly it's easy enough to change later
<sdboyer> especially in the sandbox
<msonnabaum> yeah, I just prefer it says more about what its role is when it goes in
<msonnabaum> makes it considerably easier to review and reason about
<sdboyer> sure, i'm with you on that. really what i mean about that sandbox is that it's gonna be out of peoples' eyes a bit while we work on more functionality for it, which will further clarify its role, and probably help us with the naming question
So I think we're still going to want most of our controllers returning render arrays, have an early running VIEW event subscriber drupal_render() that array into this intermediary object (flattening #attached into simple JS/CSS arrays on this object), and then a later running VIEW event subscriber turn this intermediary object into a full Response (similar to what the HtmlViewSubscriber in this patch does, but more refined).

the intention is that the B&L controllers will return one of these, not a render array. and that blocks return one of these, not a render array. for one, it allows careful control over the order in which assets are added to the stack (on a block-by-block basis), and that potentially is important to ensuring the overall ordering is correct. more importantly, though, is the page stability question: we should not *want* things to be mutating the output of blocks after the block is done with it, and having them continue to return render arrays is begging for something outside the scope of the block to reach in and mess with it. that breaks page predictability, and buh-bye caching. no - we want to return these specifically because they contain rendered HTML, which makes the block output less mutable by random outsiders.

xjm’s picture

Okay, I'm really in agreement with catch that it makes even less sense to postpone this than #1812720: Implement the new panels-ish controller [it's a good purple]... The only thing that made the VDC sandbox work is that we peeled off independent new pieces of architecture like ConfigEntity, TempStore, and list controllers and ran them through the core queue on their own before we merged Views in. (And we still accepted at the time that we were incurring a lot of technical debt, but at the time, we also had plenty of time to iterate on it before code freeze, which we've done and are still doing.) And we kept our core issue for Views itself active.

I can understand if you want to hack more on it and really represent what your code is doing, but I'm not sure "postponed" is consistent with that. The effect of setting these issues to postponed is that we're pretending they're not a part of core, and I think if that's the approach we take, we risk that they won't be. :(

xjm’s picture

we should not *want* things to be mutating the output of blocks after the block is done with it, and having them continue to return render arrays is begging for something outside the scope of the block to reach in and mess with it. that breaks page predictability, and buh-bye caching. no - we want to return these specifically because they contain rendered HTML, which makes the block output less mutable by random outsiders.

So, the first time I read this I was like "whaaaaat" and then I read it again and it made sense. It's still a render array earlier in the pipeline, right? (If I understand the Twig folks correctly, one of their strategies is to convert stuff that's being rendered too early to render arrays.) All you're saying is that this partial response should be the single final thing that happens to a block before the page or whatever is assembled. And you'd like to work in the sandbox more to demonstrate that exact big picture? (I'm still not at all convinced postponing issues and avoiding the core queue is a good idea; I just want to make sure I don't misunderstand where you're coming from.)

sdboyer’s picture

@effulgentsia - so, i read the fragment-related classes more. i misjudged them the first time around - they're all phrased in such a way that it makes sense that they return something like an HtmlFragment. so yeah, we can reuse that naming.

@xjm - it'd be dishonest to say that i'm not relishing moving to the sandbox simply because it feels like an opportunity to just write the code that, after many months of winding discussion, i'm reasonably sure makes sense. the majority of the patches i've put up have been too abstract, too incomplete, for people to grok. to that end, i do think that ignoring the core queue is the best thing we can do - for about a month. at that point, i hope we'll have some more completed segments to peel off and put back in the core queue. with any luck, that'll make what we're doing in the core queue more productive.

Crell’s picture

xjm: The idea is that the SCOTCHy controller (which replaces HtmlPageController) would call to each block it's supposed to have on that page, and each block would give back a PartialResponse object that does *not* contain a render array. HtmlPageController then stitches those together into a single PartialResponse object, which a View subscriber then translates into an HTML page. Whether or not a given block used a render array, Twig, or latex body paint to produce that PartialResonse is not the concern of the Controller.

As far as the process question of where what work gets done and in which chunks it gets committed... We have a BDFL. Tell him to come D and settle it so that we don't waste more time of 5 senior developers debating project management minutia.

xjm’s picture

Issue tags: +D8 cacheability

Tagging (though per @catch this is more the second step for the cacheability problem, but we can work on solving it in parallel).

xjm’s picture

Issue summary: View changes

total rewrite of the issue summary to match with the shifted scope of the issue.

andypost’s picture

Seems the issue is the last on in #1830854: [meta] The ESI pipeline battle plan and second already moved to d9 #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache

So why the one is postponed?

Crell’s picture

catch’s picture

Status: Postponed » Closed (duplicate)

Yes.