We're in the process of cleaning up JS, meaning that capitalized variables are meant to be Constructors, which Drupal isn't.

While moving towards more modular javascript we're going to be using the Drupal variable less (still going to be using it indirectly). Writing it in all caps discourage people from messing with it, which is a good thing.

CommentFileSizeAuthor
#31 core-js-rename-drupal-1754344-9.patch175.25 KBseutje
#8 core-js-rename-DRUPAL-1754344-8.patch175.25 KBseutje
#8 core-js-rename-DRUPAL-1754344-8.interdiff.txt280 bytesseutje
#6 core-js-rename-DRUPAL-1754344-6.patch175.25 KBnod_
#1 rename-Drupal-DRUPAL-1754344-1.patch171.85 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Active » Needs review
FileSize
rename-Drupal-DRUPAL-1754344-1.patch171.85 KB

Let's see what the testbot says.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Haha, that's awesome!

Seems to work well for me :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

sed got me most of the way there: ;)
The rest were in tests and comments

find . -name *.js|grep -v ui/ui|while read r; do sed -i 's/Drupal\./DRUPAL./g' $r;done

Status: Needs review » Needs work

The last submitted patch, rename-Drupal-DRUPAL-1754344-1.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

AjaxFramework tests failing

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-js-rename-DRUPAL-1754344-6.patch175.25 KB

got a couple more in inline JS, no idea why it's failling ATM.

Status: Needs review » Needs work

The last submitted patch, core-js-rename-DRUPAL-1754344-6.patch, failed testing.

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs work » Needs review
FileSize
core-js-rename-DRUPAL-1754344-8.interdiff.txt280 bytes
core-js-rename-DRUPAL-1754344-8.patch175.25 KB

looks like you removed a line from the top of locale_test.js and I guess it ignores the first line? (o.O)
My head hurts now :(

seutje’s picture

seutje CreditAttribution: seutje commented

went ahead and updated the summary in #1664940: [Policy, patch] Decide on JSHint configuration

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

So now the question is: when do we get this in and break every single JS patch in the queue?

I propose we wait until the jshint and the current selector clean-up issues are in to rtbc this one.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Needs work

Writing it in all caps discourage people from messing with it, which is a good thing.

That's the entire thought process behind all JS files having

DRUPAL

in them?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

This doesn't seem like a good idea to me... can you provide an example of ALLINCAPSWORDOBJECT in another project, or a reference to the standard you're referencing?

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

-1 from me too. DRUPAL is not pleasant to type. Why don't you want people "messing with" the Drupal global?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Well, I learned JS from Douglas Crockford which encourages this: http://javascript.crockford.com/code.html, the YUI library follow that, (and the old version as well, Y, and YAHOO)

Usually, all caps means it's a global variable, and a lot of JS coder will think that. The main issue here is that it's capitalized. This is not a constructor, it should be either all caps or all lowercase but can't stay like that now that we are cleaning up the naming of the rest of the things with jshint.

So well I don't really care if it's all caps or small caps, as long as it's not Drupal which is misleading.

ro-no-lo’s picture

ro-no-lo CreditAttribution: ro-no-lo commented

All CAPS look like a constant to me in *any* language.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

well, constants don't exists in JS so no chances of that happening.

corbacho’s picture

corbacho CreditAttribution: corbacho commented

I don't want to bike-shed the thread, but DRUPAL looks like a constant like ronan says. JavaScript doesn't have constants but anyway it's used like that as convention. For example Backbone.VERSION.

"Drupal" is *more* than a global variable. It's a namespace, so we could do an exception. And it's not fun to break backwards compatibility unless is necessary

I took some time to have a look to different frameworks / popular websites and check their global namespace names, you can see a pattern of having upper-case first letter:

Firebug (Firebug Lite), Modernizr, Github, Mozilla (mdn), yepnope, requirejs, FB (Facebook Button), jQuery, DISQUS, gaGlobal (google analytics), Aloha, Joomla, MooTools, Cufon, Backbone, StackExchange (Stackoverflow), Mollom, Editor ( this last one actually in the JSHint webpage )

Notice that DISQUS and YAHOO have all caps company names

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Then again it doesn't really matter, with the file-level closure we have we can name it whatever, and put Drupal as the parameter if people still want to keep using it.

First: We follow the Capitalized constructor name rule, that's in our JSHint configuration. See the relevant thread.

In the list you forgot about JSHINT, html5, google (the api, not the stats thing) and hundreds of others. And yes there are also hundreds of counter-examples. My point, we picked a standard way of doing things, this does not fit it then needs to be changed. It's weird enough trying to get in Drupal JS we don't need to make that worst.

My aim here is to get a lot of real JS devs to come to drupal. This will help, keeping the current naming isn't helping anyone and as the jQuery thing showed, people don't really care (except a handful maybe). You guys are all nice and all but if you want full offline support for Drupal in core one day, we need many more very good JS devs around to actually make that happen. And right now this change is a little piece of the problem why we don't have them with us right now.

nevets’s picture

nevets CreditAttribution: nevets commented

This seems like another change without real purpose that will break contributed modules, -1 from me.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

This will help, keeping the current naming isn't helping anyone

But it's not hurting anyone either. I write a crapload of Javascript for my personal Drupal projects and the fact that I don't have to write DRUPAL.behaviors all the time seems very nice. Also, all caps DRUPAL looks ugly and I would really hate to work on Javascript if it were that way (more than I already do).

and as the jQuery thing showed, people don't really care (except a handful maybe).

Not sure what this was. Care to share? I'm thinking that the problem isn't so much that people didn't care: it's just that they weren't aware.

You guys are all nice and all but if you want full offline support for Drupal in core one day, we need many more very good JS devs around to actually make that happen. And right now this change is a little piece of the problem why we don't have them with us right now.

Offline support? That sounds insane, but if you think it's cool, more power to you. I don't think it's a core thing though, and I really don't believe that writing DRUPAL in all caps is going to bring in more JS devs.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

As I said ealier, DRUPAL, drupal I don't care, just not Drupal.

(function ($, Drupal) {

  // Problem solved.

}(jQuery, DRUPAL));

About the offline thing it's not insane, if you think about it that's what mobile should be about, ask Damien Tournaud he'll tell you all about it :) It's not just that, we have a lot of other JS issues that need real JS people.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

If we're just going to wrap it in a Compatibility Thingy ™ anyways, then what's the point? There has been no compelling reason to make this change and two reasons against it (DRUPAL is more effort to type, and it's a needless backwards compatibility break).

I will again maintain that this change is unnecessary, it will not have the benefits that you think it will, it will annoy people, and it will not attract JS devs to Drupal.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript clean-up

This change alone won't get more people obviously, I'm looking at the bigger picture and this is a dirty spot on the picture. seutje was telling me on IRC that he got questions from devs about the naming. It's not just me or him. I'm showing the closure thing as a backward compatibility measure. If I have my way we won't even need the closure anymore but that's another story.

To be fair I hadn't tagged the issue to make it part of the bigger changes that have been happening since January.

My question to you is: how do you justify that you can't do new Drupal even if Drupal is capitalized?

(edit) the comment about DRUPAL being more effort to type is spot on, we don't really want people writing it to begin with (and this ties into our weird behaviors system). There are a lot of things connected to this, it's not just a naming issue.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

You have to justify things getting into core, not the other way around.

Look, if you're going to refactor a bunch of javascript so that a dev doesn't ever need to type Drupal.behaviors, be my guest. I might even help with it. But until that happens, this issue should be postponed, because if that other cleanup doesn't happen, writing Drupal javascript turns into a PITA with no extra benefit.

That is, fix the underlying problems, and THEN fix the naming issue and I'm happy.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

The justification for this is here: http://drupal.org/node/172169#constructors and #1664940: [Policy, patch] Decide on JSHint configuration. It's already in core and has been for a long time. It just wasn't enforced before.

I'm one of the guys maintaining this stuff, coherence is good for me and it's good for the users. Right now I can't tell people: "whatever you see something capitalized, it's a constructor you can (and must) use new on it". I'm trying to lower the barrier of entry for new people (should I say new People? :) why is that so bad especially given that we have an easy way of porting old code?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review

To answer #11 it's not just that, see above.

Kiphaas7’s picture

Kiphaas7 CreditAttribution: Kiphaas7 commented

+1 on not using Drupal. It's not a constructor, therefore inconsistent. either drupal or DRUPAL, I don't care. But I also agree that it seems a bit overkill to break backwards compatibility for this. Unless there is a greater plan behind a seemingly silly name change......

(edit) the comment about DRUPAL being more effort to type is spot on, we don't really want people writing it to begin with (and this ties into our weird behaviors system). There are a lot of things connected to this, it's not just a naming issue.

Look, if you're going to refactor a bunch of javascript so that a dev doesn't ever need to type Drupal.behaviors, be my guest. I might even help with it. But until that happens, this issue should be postponed, because if that other cleanup doesn't happen, writing Drupal javascript turns into a PITA with no extra benefit.

So what is this greater plan? What do you actually want to do here? Remove the .behaviors stuff? Sounds interesting, please elaborate/link to the relevant issue :). If this is a stopgap solution on the move to greater things, where Drupal.behaviors is destroyed anyway, you might get a lot less resistance here.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Yep I can make a write-up of my plan, need to talk that through with a few other people before making an actual write-up, but all that can only happen if #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js lands and not in a few weeks or we'll run out of time anyway.

corbacho’s picture

corbacho CreditAttribution: corbacho commented

Another point against all-caps variables: Google Closure Compiler will treat All caps words as constants
https://developers.google.com/closure/compiler/docs/error-ref

What if I place a constructor inside the Drupal namespace? Doesnt' look more readable with Drupal than drupal ?
var test = new Drupal.whatever.Constructor();

"whatever you see something capitalized is a constructor"
Well, that's not true even in native JavaScript. Math is not a constructor, it's only a namespace for a group of constants and methods...

In the other hand, I see that you want to force consistency in the code, and I respect you for that. In that case I would go for "drupal" then, because also there is a big trend specially among node.js developers to have all lower-case packages names. But anwyay if we want to remove the drupal namespace at some point, maybe all this mess is not worth it.

The big list of examples I put in #17 have "real" JavaScript developers behind, so I think that it's a pattern very familiar among JavaScript developers. Don't agree with "this is scaring JavaScript developers"

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Title: Rename Drupal global JS object to DRUPAL » Rename Drupal global JS object to drupal
Status: Needs review » Needs work

The namespace won't disappear, it'll just be kind of 'not used that much anymore' we'll get access directly to the objects contained inside.

I'm fine with the lowercase version (it's covered by the trademark same as Drupal or DRUPAL so that shouldn't change anything). Then again, this is not a node package. That would allow us to match what's going on better in hook_library_info() though, which is good.

Can someone update the patch based on #8 and the command in #3?

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs work » Needs review
FileSize
core-js-rename-drupal-1754344-9.patch175.25 KB
seutje’s picture

seutje CreditAttribution: seutje commented

Updated #1664940: [Policy, patch] Decide on JSHint configuration

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +API change

All-uppercase didn't fly at all for me, for the reasons outlined by others already, so I appreciate that's no longer being proposed.

While lowercase drupal works, I don't really see what's problematic with Drupal compared to jQuery though.

As others have mentioned, this is a quite heavy API change and will require 13,000+ contributed as well as an unknown amount of custom modules to be rewritten accordingly. That said, the upgrade path is simple. But nevertheless, we better have very good reasons for changing it.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

I'm not hard to please, dRupal works just as well, pretty ugly though. The issue is the first capital letter which kinda violate our coding standards among other things.

nevets’s picture

nevets CreditAttribution: nevets commented

Fix the coding standard then :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Closed (won't fix)

Meh.