Part of #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff. Sister issue of #2073819: [META] Remove direct calls to drupal_add_css(), #2073823: [META] Remove drupal_add_js() calls and #2168113: Add leading underscore and other discouragement to drupal_add_css() and drupal_add_js().
/Users/jbeach/code/drupal/core/d8/core/includes/common.inc:
2706: function _drupal_add_library($module, $name, $every_page = NULL) {
/Users/jbeach/code/drupal/core/d8/core/includes/common.inc:
2706: function _drupal_add_library($module, $name, $every_page = NULL) {
/Users/jbeach/code/drupal/core/d8/core/includes/common.inc:
2488: * @see _drupal_add_library()
2766: * @see _drupal_add_library()
/Users/jbeach/code/drupal/core/d8/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php:
124: // Load an admin page with JavaScript so _drupal_add_library() fires at
/Users/jbeach/code/drupal/core/d8/core/modules/system/system.api.php:
314: * @see _drupal_add_library()
| Comment | File | Size | Author |
|---|---|---|---|
| #94 | rename-drupal_add_library-2171071-93.patch | 68.64 KB | jessebeach |
| #94 | interdiff.txt | 536 bytes | jessebeach |
| #94 | interdiff.txt | 536 bytes | jessebeach |
| #94 | rename-drupal_add_library-2171071-93.patch | 68.64 KB | jessebeach |
| #91 | interdiff.txt | 536 bytes | jessebeach |
Comments
Comment #1
wim leersJavaScriptTestpasses locally, let's see if the rest passes also.Comment #2
wim leersComment #3
sdboyer commentedComment #4
jessebeach commentedI found one stray reference to
drupal_add_libraryin a comment. I'd like to update it just for consistency even though it won't affect any behavior changes.Otherwise, this patch is good to go once the bot comes back green.
Comment #5
jessebeach commentedComment #6
webchickExxxxxxcellent!
Committed and pushed to 8.x. Thanks!
Marking for a change notice.
Comment #7
wim leersUpdated https://drupal.org/node/2169605.
Comment #8
moshe weitzman commentedThis went from RTBC to commit in 1 hour! Lets try not to do that please.
I can ignore hideous code in test files, but this put some hideous code into form.inc and OpenDialogCommand.php. How is this clearer than drupal_add_library():
Is that how theme functions are supposed to add libraries? I would have expected this library to be declared in the form element definition.
Comment #9
wim leers#8: if there is a render array, I always use it. But obviously I missed that spot! We'll do a follow-up to fix it.
Comment #10
dawehnerImpressive: You just broke phpunit. It would be kind of cool if patches would always look at the code coverage, as this would run them.
The problem is basically that out testbot runs phpunit through an kind of bootstrapped drupal, which includes code, which is not included in an actual phpunit enviroment. In general I think we should get rid of the integration
and use the phpunit command for both the testbot and local development.
Comment #11
dawehnerNope, this is a bug.
Comment #12
jessebeach commentedoy, sorry for the trouble dawehner :( I've been refactoring this code today to moshe's comments in #8.
I'm not even sure how to reproduce that error you documented above.
In any case, we should get the fix in. I'll review it in an hour. I'll open a new ticket for the refactoring I'm doing.
Comment #13
jessebeach commenteddawehner, how are you invoking phpunit to get this output? I've never used the program before and I can't seem to get it to produce any information. Are you running it from the
coredirectory?Comment #14
jessebeach commentedCode cleanup followup to #8: #2173881: Followup to Remove drupal_add_library(); make the code prettier by avoiding calls to drupal_render in theme functions
Comment #15
jessebeach commentedI followed the instructions here: https://drupal.org/node/2012184
A little wrangling of programs produced the following test output on 1415f2adc9310ffd8ccadec674a72a7f5fc38b76
command:
cd core; php ./vendor/bin/phpunitphp version 5.4.17
output:
Two errors, but not the ones you saw. I'm not sure how to account for the discrepancy. Did you run phpunit with any special flags?
Comment #16
wim leers#10: WTF!? PHPUnit tests are also run by testbot, so I don't understand how testbot can say it's green yet running it locally fails?
And like #15, I get different errors, using PHP 5.3.26.
Comment #17
dawehnerThe root of the problem is that the drupal_render method is overridden multiple times, so that depending on the order of the execution of the unit tests, we might end up in a wrong overridden drupal_render method. Let's avoid this from now since the beginning and wrap all the external calls.
I can just guess why #15 has different failures, but I would assume the order of execution does matter and this might differ from different versions of phpunit (I have one installed on my computer and not use the one from core.)
That is all odd.
Comment #18
jessebeach commentedGiven that dawehner, Wim and I got different results when running PHPunit, I think we should move this issue off into its own queue.
#2174381: PHPunit throws inconsistent errors; Tests that pass on Testbot are failing locally; wrap overridden methods
There's no need to revert the issue here because tests are passing on testbot and reverting it would not resolve the locally-run PHPunit errors. This issue also unblocks further work on rendering caching.
dawehner, I'll leave as needs review and give you chance to respond.
Comment #19
webchickYeah, actually I agree. It doesn't look like this is going to be fast to sort out, so a separate critical bug is best. Thanks for catching this, dawehner.
Comment #20
webchickActually, since this introduced both a critical bug and a major task, reverted this patch for now. That's a lot easier.
Comment #21
dawehner@jessebeach
SO just to be sure, does the patch not fix all the failures for you? It does for me.
Rerolled the patch.
Comment #22
jessebeach commentedThis is PHPunit result I get against 8.x (2214ca4e)
The same as I got before this patch was reverted.
Comment #23
jessebeach commentedAnd the same results as #22 when the patch in #21 is applied.
So, if the patch in #21 resolves the PHPunit errors for dawehner, then that is a step in the right direction. But oh man is this disconcerting that we're getting different results from the test runs.
I'm running
The patch in #21 gets us back to fixing this issue and it moves us in the right direction for #2174381: PHPunit throws inconsistent errors; Tests that pass on Testbot are failing locally; wrap overridden methods.
Comment #24
jessebeach commenteddawehner, what is the output you get when you run core's phpunit?
Comment #25
dawehnerIt just works:
Comment #26
nod_@dawehner I don't see the problem then. I don't expect the JS to work flawlessly with a different version of the lib we use from the one in core? otherwise it's pointless to ship core with it.
Comment #27
webchickI'd love to get a third opinion on PHPUnit fails with this patch, in order to eliminate weird environment issues as the cause.
Comment #28
jessebeach commentedTweeted for reinforcements: https://twitter.com/jessebeach/status/423848805772312576
Comment #29
tim.plunkettComment #30
penyaskitoComment #31
jessebeach commentedGiven tim.plunkett's and panyaskito's clean runs, I decided I should rebuild my setup and make sure the PHP I'm running isn't wonky. So I installed PHP with Homebrew and ran PHPunit. It runs clean for me now on 8.x and with the patch in #21
I think we can recommit this patch with dawehner's fix.
Comment #32
dawehnerThank you for all the effort!
Comment #33
webchickGreat!
I'd like to see if Moshe has any further comments on the patch itself.
Comment #34
jessebeach commentedIf we want to make the code in the patch prettier in a substantive way, we need to first deal with:
#2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system
#2006152: [meta] Don't call theme() directly anywhere outside drupal_render()
There's an isuse (closed, but can be reopened), to deal with making the code in this patch prettier: #2173881: Followup to Remove drupal_add_library(); make the code prettier by avoiding calls to drupal_render in theme functions
Comment #35
moshe weitzman commentedI have no idea why we are wrapping drupal_render() but if everyone else thinks it is cool then fine by me. I'm more interested right now in issues relating to serving pages. This isn't one of those anymore. This issue needs a title change.
Comment #36
alexpottThis issue is now super confusing because the patch in #21 does not include all the change that was committed in #6. #21 is all about testability and the committed patch #4 is about fixing the issue.
Comment #37
jessebeach commentedAgreed Alex, sorry for the confusion. I am also very confused because I cant reliably reproduce the errors that dawehner reported in #10. Here is my test run on the patch from #4.
https://drupal.org/files/issues/remove-drupal_add_library-2171071-5.patch
I can't reproduce the error that dawehner reported. That patch in #4, as originally committed, seems fine. It passes tests as far as I can tell. tim.plunkett and penyaskito, can you be explicit about what you tested in #29 and #30?
Comment #38
penyaskitoI tested HEAD at that moment + #21.
Comment #39
jessebeach commentedpenyaskito, what do you get when you test the patch in #4?
Comment #40
penyaskitoHEAD (56d3a63) + #4:
Time: 7 seconds, Memory: 80.50Mb
OK (1453 tests, 4357 assertions)
Comment #41
jessebeach commentedHere is the patch from #4 with the test fix patch from #21. The interdiff shows the changes from #4 to #21.
I still can't test #21 because I can't reproduce the failure it is intended to fix.
dawehner, I'll need your guidance on this patch. I ran into merge conflicts with #4, so I need to alter it.
I can't find an issue for this @todo from the code in #21. We need to log this issue or link it here.
@todo Remove once drupal_render is converted to autoloadable code.Comment #42
alexpott@jessebeach the reason you can't reproduce the error is that it was fixed in #2173171: Core PHPUnit test AjaxCommandsTest fails on PHP 5.4 :)
Comment #43
jessebeach commentedAh! Phew, that's good to know. The inconsistency was driving me a little nutso.
I'm still not sure though if #2173171 addresses the test run errors that dawehner saw. I'd generally love to get his input again on this issue. Hopefully everyone's machines are undergoing the same behavior again.
Comment #44
jessebeach commentedIn order to clean up the use of
drupal_renderin the theme functions that moshe objected to in #8, we first need to ensure that thethemefunction is taken out of the public API. If module developers calltheme()directly for elements such astable, then they will not get the necessary attachments such as the responsive and select JavaScript files unless these are attached withdrupal_renderin the theme function, as the patch here originally did.We have an issue to rename
themeto_themeto remove it from the public API. #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system.Even better would be to remove instances of invocation of
themefrom core altogether, which is currently being addressed in the sub-issues of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().I've logged the remaining instances of
themein core to #2006152 and InternetDevels has jumped into supply patches.Comment #45
xjmSimple rebase-reroll; patch no longer applied.
Comment #47
moshe weitzman commentedComment #48
gábor hojtsy45: remove-drupal_add_library-2171071-45.patch queued for re-testing.
Comment #50
gábor hojtsyRerolled. The form.inc tableselect hunk did not apply anymore, but that was already converted to #attached (in a better way in fact). The rest applied at most with some offsets. Lets see what will this bring from testbot :)
Comment #52
gábor hojtsySo this again produces the errors in #22. I am not clear on how this was resolved above and what may cause it. We are not changing code there :/
Comment #53
longwaveNot sure how this ever passed before, as there was a wrong variable name used in TestViewAjaxController::drupalRender().
Comment #54
jessebeach commentedThanks longwave!
Comment #55
gábor hojtsyLol, that makes sense. Looks good to get in then? :)
Comment #56
gábor hojtsyAssigning to @dawehner for feedback.
Comment #57
jessebeach commentedComment #58
jessebeach commentedI'd still like us to remove the calls to
drupal_renderin the theme and preprocess functions that are processing attachments back topre_renderfunctions. We shouldn't be adding assets in the theming stage.Comment #59
dawehnerI try to understand why we set 'defer' here? What does this allows us to do?
Isn't it a total valid usecase to do that, especially for custom theming?
Comment #60
jessebeach commentedDefer downloads and executes the script on the DOM content event. Why this test needs that distinction I do not know.
Thinking about it more we actually want to move the assets attachments to a post render cache function. If the theme function output gets cached then those attachments in the theme function won't get processed.
Comment #61
mikeytown2 commentedI find this page useful when understanding what defer and async do
http://peter.sh/experiments/asynchronous-and-deferred-javascript-executi...
In advagg_mod there is a setting called "Add the defer tag to all script tags". I had to do some magic to make jquery_update's fallbacks work correctly. Also needed to make sure
jQuery.extend(Drupal.settings, ....was ran after misc/drupal.js is loaded. Just some things to look out for if going this route by default :)Comment #62
sunI wonder what we actually gain from the proposed change of this issue?
As long as we do not have a proper solution for
drupal_render(), we're just converting one global state to another global state.I'm afraid this doesn't make much sense to me. I actually think it makes the situation worse, because we'd have even more manual calls to
drupal_render()that should not exist in the first place.Instead of doing this, can't we start to inject a HtmlFragment into
drupal_render()and_theme(), so that we can properly collect the attachments for the HTML snippet?Should be attached to the 'timezone' element.
Somehow I fail to see how all of these are an improvement?
Needs work at least for the 'timezone' element.
Comment #63
jessebeach commentedAs far as I understand, calling
drupal_add_librarydirectly is circumventing caching operations indrupal_render. By pushing all library inclusions throughdrupal_render, we at least remove exceptions to the render pipeline in core.I agree that calling
drupal_rendermanually as the patch does is numerous places is not ideal, which is why I don't think this patch is ready yet. We can move most of those direct calls to a build step and allowdrupal_renderto process in the natural order.I'm not familiar with this pattern yet. Is there a place in core where we're doing something like this already that I can read through?
Comment #64
catchThe direct drupal_render() calls at least allow us to change the collection of assets internally within drupal_render(), with no API change. See for example #2168111: Allow drupal_render() to pass up #attached to parents which summarizes the discussion at Prague and has a couple of options on how to do that.
We do need to get rid of all the places where it's called directly, especially in preprocess, but it's not completely pointless getting rid of the direct drupal_add_*() calls in the interim.
Comment #65
jessebeach commentedUpdated.
I've done the following in the attached patch:
drupal_pre_render_tableand out oftheme_table.drupal_renderwithintemplate_preprocess_htmltodrupal_pre_render_html(introduced here).drupal_renderfrom after_theme()processing (but oddly before#theme_wrapperprocessing) to before_theme()processing starts. Why you ask? Well, if we process attachments aftertheme_htmlhas run, then the string for the page is already generated and the attachments we add after are not pulled into the html page string generation (which has already happened). Honestly, it seems better to process attachments before string creation starts.#statesare processed before theming and building in#pre_render. Attachments feel like another "build" phase before the stringification starts.What I did not change is the call to
drupal_renderinviews_preprocess_page. The code in this function relies on the fact that contextual module has run through its build steps first. Honestly, again, it feels like this behaviors in this function should be running in a post render cache step and I think we're going to have more work to do in Views to make them cacheable like entities now are. So I left this code alone.The calls to
drupal_renderin the tests seem to have good reason. The tests seem to be exercising attachment inclusion and thus need to process attachments on the spot to assess this behavior.Comment #66
jessebeach commentedThe interdiff didn't come through on #65.
Comment #68
joelpittetJust referencing #1939008: Convert theme_table() to Twig because it will need a re-roll after the responsive array attachemnts move of #65.
Comment #69
jessebeach commentedDrupal\user\Tests\UserAdminListingTestfailed because it was callingtheme_tabledirectly. I changed the#themeto#typefor this element and then for the rest of the'#theme' => 'table'elements in core, since really, they shouldn't be callingtheme_tabledirectly with these changes now either.Drupal\views\Tests\ViewAjaxTestis trickier. The AJAX settings command fails to inject the settings into the response because they settings are added in the theming phase...after the now-moved call todrupal_process_attachedhas run. If I putdrupal_process_attachedback where it was before, after the theme functions have run, this test passes. BUT, if I do that, that means I need to calldrupal_renderon attachments intheme_htmlsince these attachments need to happen before the HTML string is rendered.So, I'd like to solve this problem by alter the AJAX pipeline to include its attachments (e.g. commands) before the theming process starts. But after a couple hours of poking around, I can't figure out where the best place to intervene is.
I'd like to know if I'm completely off on a tangent here. We can "resolve" this issue by simply moving the call to
drupal_process_attachedindrupal_renderback to its post-theming location in the processing order and then moving the calls todrupal_renderback intotheme_html. But that doesn't feel clean to me. And I think I'm surfacing subtle troubles in the AJAX rendering pipeline that need to get straightened out. Also, why was the only failed test for AJAX settings inclusion in Views and not in the AJAX render pipeline tests as well? Seems to be a testing gap.Comment #71
jessebeach commentedRerolled.
Comment #73
wim leersAFAIK because it was never officially supported, Views simply used that loophole — I know I've had to do similar things in D7 contrib.
I think everything you've said & argued makes sense. Maybe we can take a look together at the Views code later today and figure out a different place to attach things? :)
Comment #74
jessebeach commentedAlright, this might be a huge hack, but it fixes the failing test for me locally. I'm not sure if moving the attachment of assets from
views.theme.inctoViewAjaxController.phphas the same coverage. I definitely need a Views maintainer to weight in on this. What I have seem to have done is moved the inclusion of assets in the building of the View to a step prior to theming.Comment #75
jessebeach commentedTrying again from #71. I've put the code to add the Ajax JS library and settings about the View to a new pre_render function:
views_views_pre_render.The set of conditions around the code --
if ($view->ajaxEnabled() && empty($view->is_attachment) && empty($view->live_preview))-- should keep it from being added to non-ajax views. The Feed plugin explicitly sets ajaxEnabled to false by default:Comment #77
jessebeach commentedDerp, forgot to delete that code that I copied and moved from
template_preprocess_views_view.Comment #78
jessebeach commentedMoved the inclusion of the HTML5 Shiv library from the newly-introduced
drupal_pre_render_htmlto the html element info declaration. This allows module/theme authors to knock-out the library inhook_element_info_alter, which could satisfy calls to remove it by default in #1993334: Add HTML5shiv to Stable and Classy only .Comment #79
wim leers#75/#77: Looks good, but you *really* confused me:
This is NOT a
#pre_rendercallback", it's an implementation ofhook_views_pre_render()!:)
Comment #80
jessebeach commentedDocs update from #79.
Comment #81
dawehnerAs I got confused about that.
Comment #82
wim leersComment #83
chakrapani commentedPatch at #80 no longer applies. Working on a re-roll
Comment #84
jessebeach commentedGreat! Thanks chakrapani!
Comment #85
chakrapani commentedHere we go.
change is that simpletest.theme.inc has been already cleaned up in the latest head and no longer have any instances of drupal_add_library().
Reg Interdiff:
As this is a re-roll, its not an actual interdiff.
But the only change was to remove the changes related to simpletest.theme.inc file
Comment #86
jessebeach commentedThanks chakrapani! I reviewed the changes. The function
theme_simpletest_test_tablewas removed in 5fb617d7ac1335c9a5cefddc1bba11c204f4a5ed which invalidated changes in #80.I found one more stray mention of
drupal_add_librarywithout the preceding underscore in a comment. There are no behavior changes in this patch.Comment #87
jessebeach commentedReroll. One hunk in
theme.incno longer applied.Comment #89
jessebeach commentedWim Leers pointed out that #2203407: Replace #attached library array values with provider-namespaced strings got committed this weekend which lead to a ton of conflicts here.
I reviewed line by line after rolling. It would be great to get a second thorough review.
Comment #91
jessebeach commentedIt seems I missed #2184653: Rename Drupal\Component\Utility\Url to UrlHelper from 5 days ago as well.
Comment #92
jessebeach commentedgo bot go
Comment #94
jessebeach commentedAlright, this issue form is not putting me in a good mood. Try it again.
Comment #96
jessebeach commented#94 is proof that testbot is a quantum state machine.
Comment #97
jessebeach commented94: rename-drupal_add_library-2171071-93.patch queued for re-testing.
Comment #98
jessebeach commented94: rename-drupal_add_library-2171071-93.patch queued for re-testing.
Comment #99
moshe weitzman commentedI reviewed this carefully and couldn't improve the bits that I found ugly. So, progress!
Comment #100
joelpittetRTBC++
Thanks for doing half the work for each issue in this meta #1876712: [meta] Convert all tables in core to new #type 'table', they will need a minor re-roll. Just a note for anybody working on #type=>table conversion issues #rows key will by-pass how the child elements get built so that still has to be addressed in those issues.
Also this will need a re-roll: #1939008: Convert theme_table() to Twig
Comment #101
webchickAll right, this has sat here almost an entire day now, and the person who previously objected now marked it RTBC, so I hope it's safe to commit it this time. :) Testing that theory!
Committed and pushed to 8.x. Thanks! Awesome work, all!
Comment #103
tetranz commentedThis issue added these classes with @inheritdoc on them. while looking into writing actual docs (since we don't inheritdoc classes), it seems like maybe they should be stubs instead. See issue #2250165: Replace fake mocks with actual OpenDialogCommand stubs in AjaxCommandsTest.
Comment #104
jibranCan we remove this @todo now after #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls?