String freeze: make site_name's default value consistent

I have supplied a patch that introduces a new define() in bootstrap.inc to create a constant for the default value. In a recent dev list thread I believe someone joked at this method as being "variable constants". But in this context it makes sense to me at least. It defines what the default value for the site_name should be. The patch then applies it across Core. Note, there are a few "exceptions" and in those cases I have added a comment to show that the default value is not really what's wanted in those specific cases.

Note the issue version change to 5.x-dev. I believe all issues like this should start at "HEAD" and, if appropiate, be backported to earlier versions.

I should have mentioned, I'm not entirely sure this is a "bug" but I'll leave the issue category alone. Potential reviewers can set/reset this appropiately

Hm. I kind of think that's overkill, and results in way more typing. There are only maybe half a dozen places in core where this is not consistent. This patch changes those instead.

There were a few I didn't touch:
- the variable_get('site_name') in the drupal_notify($server) function.. no idea what that function does, but all of the variable_gets there default to '' so I figured I'd leave that one alone.
- the variable_gets('site_name')s in ping.module.. one is in an "if" statement, so putting 'Drupal' there would screw it up. The other is another of these _notify functions.

Also changing status to critical task, to match the other string freeze-related issues.

I kind of think that's overkill, and results in way more typing. There are only maybe half a dozen places in core where....

Thanks for the review. Hmm to myself indeed, I hope you don't mind me asking this but if that sentence could be reduced to two words it would be an oxymoron (as in if it doesn't appear that often how is it "way more typing"?).

You won't mind if I step back for submitting core patches in future as I find the level of peer-review to be inconsistent. Either there's a coding standard or it's lacking in some way. I myself would say your fix was "hackish" and leaves the door open for others to make the same mistake (afterall, a number of people already did make that mistake and they all passed peer-review)

Hmmm?

I just meant:

variable_get('site_name', 'Drupal')

is easier to type than

variable_get('site_name', DRUPAL_DEFAULT_SITE_NAME)

and that it's also easier to remember "the default for site_name should always be 'Drupal'" (unless there's a compelling reason not to do so, as in ping.module) than it is to remember the name of the constant when typing this code in a module (was it DRUPAL_DEFAULT_SITE_NAME or was it DEFAULT_DRUPAL_SITE_NAME or was it DEFAULT_SITE_NAME or...?).

I certainly didn't mean any disrespect. :\

My feeling is that the reason it was inconsistent before was that it never was consistent in the first place. This patch would make it consistent, so it would be very easy for someone who wants to place a call to variable_get('site_name') in their own module to grep core real quick and learn what the default value should be.

Just to provide a different perspective, there has been some work towards removing "Drupal" from user visible places. For 5.0 we all agree that there are likely to be a blossoming of distributions. For a distribution to work they need to have a consistent brand. If that branding means they will have to go through the code and maintain a patch to replace variable_get('site_name', 'Drupal') with variable_get('site_name', 'DistroName') then we'll be taking a step backwards.

What about using something that is neutered like 'Website'?

If others agree to the concept I'm happy to re-roll the patch.

Hm. I'm not too keen on that idea... for 90%+ of people who are installing Drupal, it should say "Drupal"

For distributions, a simple variable_set('site_name', 'Super Distro 2000'); in a profile_name_install() function will do the trick for making sure that "Drupal" never shows to the client (at least from this variable). 'Drupal' is only displayed if this value isn't set.

The details of an implementations shouldnt be shown to the end user in my opinion. Genericizing it to the degree possible is a feature for future distributions and private label users who dont want or need it known that drupal is running under the hood.

We use clean urls to provide memorable and implementation agnostic addresses and i think this is a natural extension of "drupal as a tool not a destination". As much as I like to see it everywhere I just don't see a valid reason to be flashing "Drupal" around on everyones sites.

Maybe for distros they should be looking to insert their preferred values into the variable table during the install? They can have what they like there. A "do you SQL bit now" type hook during installation (if one doesnt exist)?

We were hashing this out on IRC more...

It doesn't really make sense to change the default value of site_name without also going through and removing all other user-facing instances of the word "Drupal." That is much too big of a task to undertake at this stage of the game, so a postponed task was created for this here: http://drupal.org/node/98905

Distro authors have the option of doing a variable_set('site_name', 'Foo') in their profile_name_install() function to ensure that the site name variable is never set to "Drupal" (unless the user does something crazy like truncate their variables table :P). I believe this to be a suitable workaround for the Drupal 5.x release cycle, and we can revisit #98905 if there is enough call for it.

I agree with what webchick said in #11.

What about making the second parameter of variable_get() optional (NULL as default)?

Then, we could do variable_set('site_name', 'Drupal'); in the default install profile and just use variable_get('site_name'); everywhere we need the site's name as we can be sure that site_name is set.

Sure, that would also work. Care to write a patch for that? I'm about spent on this stuff.

Ok, here's the patch.

It makes the default value parameter for variable_get() optional with NULL as default, sets the 'site_name' variable during installation (in the hook_install in the default profile) and changes variable_get('site_name', whatever) at all places specified in patch #3 to variable_get('site_name'), as we can now be sure that 'site_name' is always set.

Cool. I think this will end up being the best solution. A couple things, though:

1. You're changing an API with your $default = NULL in variable_get.. that's a no-no, this late into the code freeze. So instead, will need to change the variable_get('site_name')s to variable_get('site_name', NULL)

2. Your settings.php file got included in the patch...

1 - changed this. Might be an option for 6, though. Would save us some typing.
2 - thought I excluded it ..

new patch is attached

1. "Has anybody else looked at any of the other variables to see if their are any more inconsistencies?"

No, because the focus is on the string freeze, so we're only worrying about user-facing text. theme_default (and most others) doesn't qualify.

Please open a separate issue if you'd like to explore that. However, with most variables like that, you definitely don't want the default to be NULL. If something happens to your variables table or something, all of a sudden all you see is a completely unstyled page with no blocks, including the navigation block. Try setting your theme default to something, then changing the name of the theme's directory and you'll see what I mean.

2. "Then this would be a lot less typing."

Yes, I agree. Either you or Frando or someone should probably open a 6.x-dev feature request for it. But it may not be accepted for the reason I outlined above.

In any case, can we please keep this issue on-topic for the string freeze only and the site_name variable in particular? We have to get all these patches in before the RC or else we can't touch it again until 6.x-dev.

Your proposal is an API change and thus won't make it into 5.0, but might of course be considered for 6.

However, IMO it's easier to just set all default vars in hook_install (as my patch in #18 does for the 'site_name').

And in 6.0 we can still make the second parameter of variable_get() optional.

Review of the patch:

- Chameleon.theme's site_name got missed.
- There are other instances of site_name you'll want to change as well... the ones in ping and drupal module; basically, all of them should read the same.
- There is going to be some awkwardness for people who are updating from a head site (the Drupal help text will read: "here at with a" but we generally don't care about HEAD updaters so I don't think this issue is worth worrying about.

I think this is flawed. We can't make the default parameter optional because we can't assume variables to be set. Changing that behavior looks like a really bad move. It should be possible to wipe out your variables table.

Ok. In that case, here's a re-roll of #3, which simply makes the default value 'Drupal' everywhere, so it should be ready to go.

Committed to HEAD (as soon as my wireless stops being flaky).

I'm sorry, but why do we want to use 'Drupal' instead of t('Drupal') for the default value? Maybe in one of the non-Latin languages they want to have their locale change 'Drupal' to something else right off the bat. I know they can eventually change this in settings, but I feel like we pass everything else through t(), why not this?