Problem/Motivation
system.js is full of old and ugly code that we can replace with Drupal form states and form ajax. Beside Drupal.behaviors.pageCache() in system.js file do not work (even for D7).
Proposed resolution
Instead of using Drupal.hideEmailAdministratorCheckbox() and Drupal.behaviors.pageCache() with ugly selectors we can use Drupal form states - much cleaner and assumed way how this things should be done for D7 and D8.
Instead of using custom Drupal.behaviors.dateTime() for getting example date formats over ajax when adding/editing date formats we can completely remove this custom js code and use Drupal form ajax which is again right solutions for D7 and D8.
All this changes will also make system.js file lot smaller and now system.js is used only by installer, so we will also have some performance benefits on server and client side.
Remaining tasks
We should probably port this changes to Drupal 7 also.
User interface changes
None.
API changes
None.
Original report by David_Rothstein
Looking at system.js, every single function in there appears to be something that runs only once, on a single page in Drupal. Because of this, it would be much better to split it up into multiple files, intelligently named, and only include the particular file that is needed each time.
This would lead to more readable code and allow removing some of the obscure jQuery selectors in that file. It would also mean that crazy bugs like this one - #479604: Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly. - would no longer be possible.
Comment | File | Size | Author |
---|---|---|---|
#53 | core-js-refactor-system-492720-53.patch | 10.4 KB | nod_ |
#53 | interdiff.txt | 1.72 KB | nod_ |
#50 | core-js-refactor-system-492720-50.patch | 9.68 KB | seutje |
#50 | core-js-refactor-system-492720-50-interdiff.txt | 2.69 KB | seutje |
#41 | core-js-refactor-system-492720-41.patch | 9.61 KB | nod_ |
Comments
Comment #1
JamesAn CreditAttribution: JamesAn commentedSplitting up system.js sounds more appropriate to me too, since the functions are for specific system pages, rather than for the whole module.
Here's the list of the js functions and which PHP function build the relevant page:
Drupal.behaviors.copyFieldValue may still be counted as a utility function. Even though it's currently only used by install.php, it can be used elsewhere. I didn't move that function in the following patch, but all the other ones have been moved to their own js file.
Comment #3
sunWrong. That bug is caused by a JavaScript bug and nothing in Drupal prevents a JavaScript from loading. Even if system.js gets splitted, a buggy JavaScript can still be loaded and break other functionality.
I'm not sure the proposal of this issue makes sense. As of now, system.js is only loaded conditionally where explicitly needed - and that's rare already.
Comment #4
sunHowever, it perhaps would make sense to introduce a new misc/install.js file to bundle all the special logic for install.php.
Comment #5
sunTagging for feature freeze.
Comment #6
JamesAn CreditAttribution: JamesAn commentedThe tagging never happened. Is this still a go for D7?
Comment #7
sunSince this would just move code around, and even could count as performance improvement (since that JS for install.php would no longer be loaded/aggregated), I think so, yes.
I'm still not sure whether it should be misc/install.js or modules/system/system.install.js. I think the latter would be cleaner.
Comment #8
nod_Comment #9
pivica CreditAttribution: pivica commentedtaking this one.
Comment #10
pivica CreditAttribution: pivica commentedHere is a new patch, at the end instead of splitting system.js to multiple files I refactored that old/ugly js code in it - most of the stuff we can do now with states and form ajax. On the end we have only one behavior in new system.js which is used for now only by installer.
I need a review of changes in system_date_time_lookup() - had some problem with replace state - it is injecting one additional empty
<div></div>
on the beginning for some reason at that is messing layout a little bit, so I used ajax_command_replace() but maybe there is a smarter way for this.The rest seems ok, tested in FF, no need to test in other browsers (IE) because there are no changes that are browser specific.
I also runned jshint on new system.js and applied recommendation from it, this change (and whole refactoring of system.js) is related to #1684872, so i guess this two patches should be merged into one.
Also we should mybe backport this to D7, because Drupal.behaviors.pageCache does not work in D7 also.
Comment #12
pivica CreditAttribution: pivica commentednod_ explain it to me why we should use function someFunction() instead of var someFunction = function(), so here is new patch with that change. But we still don't know why install is failing with testbot.
Comment #13
seutje CreditAttribution: seutje commentedIs there still a point to this "Add JS to show / hide ..." comment line? Attached patch removes that.
Also: no clue why it fails on MySQL during install, seems unrelated to these changes. So, gogo testbot!
Comment #14
pivica CreditAttribution: pivica commentedYeah you are right we need to remove that comment also.
Comment #16
seutje CreditAttribution: seutje commentedI really suck with testbot, and have no clue how to debug this.
Can anyone confirm this isn't PIRF tweaking out?
Comment #17
nod_Might be related to the change of checkbox name, any way to keep the old values of the checkboxes?
Comment #18
pivica CreditAttribution: pivica commentedNo I don't think so, because we changed checkboxes to 2 checkbox - which we need because we want to use states on that two checkbox. I guess install test for bot is hardcoded somewhere else and not in drupal code - somebody need to change that install test script which will be bad because all other issues test will fail until this patch is applied to drupal.
I don't know what is a resolution process for this kind of patches - any idea?
Comment #19
nod_What I meant was it should be possible to do, have a look into form_process_checkboxes() in form.inc an try to manually do that.
As i've learned the hard way, it's unlikely to be the testbot that fails :)
Comment #20
pivica CreditAttribution: pivica commentedOkey here is slightly modified patch from 13 - same stuff basically but bot will maybe happy with this, lets wait and see...
Comment #21
nod_Awesome :)
a few things and it'll be good to go:
The
updateTarget
function needs a parameter e so that you can refer toe.target
instead of usingthis
line 19.There is a semicolon that needs to be removed line 23 as well.
You can remove
context
in the jQuery selector since we're looking for an ID.That is a very very nice clean up :)
Comment #22
seutje CreditAttribution: seutje commentedFixed JSHint objections and removed context as per #21
Comment #23
pivica CreditAttribution: pivica commentedOK here is new patch based on hints from 21. Just a short question, can we use e.target.value to get value of input or we need to stick to $(e.target).val(). For now i did it with jquery but e.target.value is shorter - but not sure is it ok for all browsers.
Also I would like that someone with more states experience check
I mean it's ok that we use ajax_command_replace to avoid empty div problem but maybe there is more cleaner way?
Comment #24
nod_ok for me.
Comment #25
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #26
pivica CreditAttribution: pivica commentedAdded issue summary and changed issue title.
Comment #27
pivica CreditAttribution: pivica commentedForgot to remove need issue summary tag.
Comment #28
nod_This patch makes me happy :)
The #state thing is weird but that's what you got to do to make it work. We need a better ajax.js to fix that.
Comment #29
tim.plunkettThese numeric indicies are really hacky. Why are they needed? If the actual numbers are important, they should be comments.
Otherwise, this looks good.
Comment #30
nod_it's kinda manually expanding checkboxes, that's what it'd become after form_process_checkboxes runs on it. Doesn't look great but looks valid to me.
Looking at it again It is missing the
#return_value
key for the individual checkboxes though.Comment #31
pivica CreditAttribution: pivica commentedComment in patch already saying it all
So we are using numeric indicies to make testbot installer part not breaking. Original patch from 10 used normal text keys here but testbot was failing with them - I think that a problem is in testbot installation script - it needs that numeric keys here because original installer was using checkboxes with numeric keys - bad!
I agree this is ugly and hacky, but we need to change testbot installer part also then in order to make this more pretty.
Added #return_value and rerolled patch against latest dev.
Comment #32
sun1) I'm not sure whether there isn't actually another issue for this already, but I'd really love to replace the numeric indexes with more meaningful keys; e.g., 'check' and 'notify'.
2) You do not have to switch to #type 'container' to achieve what you're doing. Instead, declare the #type 'checkboxes' as it was before, and then prepare the addition to the second as follows:
That works, because form_process_checkboxes() makes sure to merge any predefined properties into the expanded sub-elements. :)
Aren't these the defaults?
I wonder whether the code shouldn't use the actual HTML ID of the element as defined in $form instead of hard-coding #edit-date-format-suffix?
Defining this as a function within the scope of .attach only is pretty confusing... I don't quite get how it is used/triggered...?
Lastly, some of the comments mention "ajax" and "api". The former should always be "Ajax", and when referring to Drupal's "Ajax framework", just use that term instead of "API" (always uppercase). :)
Comment #33
seutje CreditAttribution: seutje commentedattached patch only addresses concerns regarding system.js, haven't touched the other concerns, so I'm leaving this as "needs work"
It felt silly abstracting a behavior that simple to a proper constructor, so I just abstracted the handler and put it on the behavior object.
Not sure if the internal mapping makes sense, but this would make it possible to call this behavior with settings that aren't present in Drupal.settings without asploding those settings.
Comment #34
nod_All right, took another approach in the JS, hopefully that'll be clearer what's going on.
Changes the form stuff with sun suggestions. The #ajax throbber array is needed, otherwise there is a message, to remove the message and keep the spinner image, it needs to be like that (otherwise there is nothing showing up).
Can't use the ids from the ajax callback since drupal_html_id just gives crap results we can't use when you're in the ajax response.
Comment #36
nod_Seems like a testbot issue, can install Drupal with and without JS activated on my laptop.
Comment #37
sunSorry, my fault. The suggestion to change the key names in install_configure_form() does not work, because that requires changes to PIFR.
Re-rolled without that. Didn't test, but looks RTBC.
Comment #38
pivica CreditAttribution: pivica commentedSmall error in # 37 patch
instead of 'check' and 'notify' we should use old [1] and [2] keys. Beside that everything else looks good and I did full test of newest patch.
Comment #39
webchickWe talked at length in IRC about large refactoring patches like this, and their likelihood to break things in core. We decided to tag issues like this with "Needs JS testing" and then zendoodles and her Merry Band of Contributors will test these patches manually (for now) and with automated testing (later!).
Tagging so she can find it. :)
Comment #40
nod_Test updated http://drupal.org/node/1777342 + reroll
Comment #41
nod_reroll. Tests were added to the testing page http://drupal.org/node/1777342 and everything is working as intended.
Comment #42
cweagansJust looking at the code, it all seems pretty sane. I don't want to RTBC it, though, because I haven't actually tested it on a live Drupal install. It looks to be a very nice cleanup, though. Thanks!
Comment #43
sunOverall, this looks pretty good, but I have some concerns:
1) Shouldn't the event name be namespaced in some way? "value:copy" looks like it has a high chance for running into unintentional name clashes with arbitrary other libraries that may run on the page.
2) It's the first time I see a colon in a event name. Why do we do that?
3) A more descriptive event name would be better; e.g., copyValueFromSource
1) This comment leaves the reader in the dark for as to "why."
2) Why is this behavior completely ignoring context? That seems to be the root cause for why the code needs to explain internals about jQuery events, which it shouldn't have to.
3) Closely related to that, wouldn't it make sense to provide this behavior as a standalone file/library in /misc, so that modules which want the same behavior can simply load and apply it to their forms?
Comment #44
nod_1, 2) have a look at states.js :)
3) if you have a better name than
value:copy
sure, I just wanted to keep the naming consistent with #states since it works the same way.4) Because of jQuery. I don't think the comment should be used to explain jQuery internals. Sizzle can't access the document object so it can't be used to bind events and use the bubbling propriety of the events. I don't remeber the details right now but I tried with document, it didn't work and looked up how events were managed in jQuery to find out what I wanted to do wouldn't work. states.js is using document but it's using events in a rather convoluted way.
5) It's ignoring the context because we're working with IDs and because we're binding only one event since we don't need more than one event listener for all this.
6) I'm not sure that's wanted. It's pretty short and once you get how it works it's pretty simple. We can make things short and simple here since we can rely on the fact that we're using IDs all over the place. It's not the case for #states and would introduce more code for the same result.
Comment #45
nod_Back to RTBC, all it was missing was tests in #39, they've been added and patch works.
Comment #46
seutje CreditAttribution: seutje commentedWhy are those event handlers not being defined as
Drupal.behaviors.copyFieldValue.valueSourceBlurHandler
? That way, it would be rather easy for 3rd party scripts to override it if they wanted to, right now, they would either need to override the entire behavior, or just swap out the file altogether...Comment #47
nod_the file is really small now, so if you need to override one part it's just easier to override the whole thing. Loading dead code shouldn't be our aim as an override mechanism. I'd rather have everything as small as possible and separated in it's own file than doing monkey patching of big files.
Comment #48
seutje CreditAttribution: seutje commentedTrue, but having it on the behavior object wouldn't increase the file size by much, would it? just the extra bytes for indentation afaict...
Comment #49
nod_feel free to reroll, The main problem with the issue is that Drupal doesn't use it's own API, that's small stuff I don't care much either way :p
Comment #50
seutje CreditAttribution: seutje commentedI don't really care that much, that's why I didn't change the status but I can imagine that when some1 is debugging this, it's nice to have a reference-able function that isn't hidden inside a closure
Comment #51
nod_works for me, thanks.
Comment #52
nod_adding a few things, hang on for a new patch.
Comment #53
nod_Some dependency things :)
tests added, removing tag
Comment #54
seutje CreditAttribution: seutje commentedoh, right :P
Comment #56
nod_JS only patch, testbot failure irrelevant.
Comment #57
seutje CreditAttribution: seutje commentedyeah, I don't rly get how it could fail before that page
stepped through the installer to make sure and it works as expected... e-mail was properly copied over
Comment #58
catchLooks like good clean-up. Committed/pushed to 8.x.
Comment #59
andypostIssue summary points that we need backport
Comment #60
sunThis would be an API change — and worse, in some code that isn't tested well.
Comment #61.0
(not verified) CreditAttribution: commentedby pivica - added issue summary.