Problem

Drupal currently defines constants using the define() function. Since up to 500 constants can be defined by some Drupal installs, this can up add to around 1ms of CPU time and a measurable amount of memory to each full bootstrap.

Proposed solution

This issue went through various solutions:

- Using class constant and lazy loading the classes.
- Allow constants to be optionally defined via PECL extensions that exist to workaround this issue (apc_load_constants(), hidef etc.)

However, now that Drupal 8 has a PHP 5.3 requirement, we can easily switch to using the 'const' keyword to define constants. This allows constants to be defined on compile time instead of every request, is a significant improvement in CPU usage, and it looks like a small memory improvement as well (although that may be down to the way xhprof measures memory usage).

Original report
working on #332003: cut the fat out of _drupal_bootstrap_full, it became clear that lazy-loading needs contants defined by classes.

if we have my_module::MY_CONSTANT, lazy loading via the registry will Just Work, which is nice.

IMHO, its also nice from a DX point of view.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Another approach would be to place constants inside .module files, and load those unconditionally. I think it will prove useful to be able to place there functions that will be always available, even if you don't call drupal_function_exists() before.

I'm not convinced either way, for now.

Anonymous’s picture

@Damien: i'm not 100% convinced either, but i'm favouring the 'just lazy load all module stuff' right now.

Anonymous’s picture

Status: Active » Needs work
FileSize
37.17 KB

in #drupal IRC, webchick asked for some examples of what this might look like.

so, attached is a patch that does the simplest possible implementation for comment.module's constants.

i'm in no way wedded to the details here, just wanted to give a possible example. the implementation is trivial, as its just some grep's and find replaces in vim.

is this general approach viable? should we put the class in a separate file? how should we deal with constants that don't clearly belong to a module?

catch’s picture

We could have module.constants.inc maybe?
Stuff that doesn't clearly belong in a module usually gets stuck with system.module

I'm neutral on this for DX - neither better nor worse IMO (although a constants file would be nice maybe) - if it makes registry lazy loading during bootstrap easier then it's worth doing though.

chx’s picture

yes it would be great to autoload constants.

Anonymous’s picture

well, that's enough support to make a real patch worth the effort i think.

i'll go with catch's suggestion of module.constants.inc - seems pretty obvious and self-documenting.

Crell’s picture

I'm -1 here, for reasons mentioned here: http://drupal.org/node/332003#comment-1114968

Anonymous’s picture

Status: Needs work » Closed (won't fix)
catch’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (won't fix) » Needs work
Issue tags: +Performance, +memory

This seems reasonable to me.

My eyes glaze over every time I open a .module file and have to scroll past dozens of constants. They also show up as a write when stracing PHP, although I don't think it has any measurable impact, a site with a lot of contrib modules can end up with hundreds of extra constants defined.

We also have code that calls get_defined_constants() and eats memory like crack - see http://drupal.org/node/281405#comment-2378224 for an example.

catch’s picture

I took a look at how many constants we define.

To get a baseline, ran this in the browser on my localhost to see what PHP plus extensions are defining:

print count(get_defined_constants());

Result was 1489.

Then I checked immediately after a full bootstrap in Drupal 7 (more or less standard install), this gave me a count of 1671. In xhprof, there are 181 calls to define(), taking around 300us (yes that's nothing) and 31,424 bytes (around 30k of memory).

Then I did the same for a Drupal 6 site with > 100 modules. 400 calls to define() taking 624us, and 97,056 bytes of memory.

Then I stuck a call to get_defined_constants() at the end of _drupal_bootstrap_full() in the D6 site again, and profiled the same request with just that one call. The memory use for a single call to get_defined_constants() was 452,208 bytes - so worst case probably half a megabyte. Since we use this in drupal_parse_ini_file(), this could well be taking us over the edge on some requests. The same call at the beginning of _drupal_bootstrap() full was 363,912 bytes - so 90k of memory less before includes and modules are loaded - this fits with the 97k from earlier given we define some constants before full bootstrap too.

In the worst case, our use of define() is going to take at most 1ms, and 100k of memory, it would be nice to claw back some of that memory if it's possible. 1ms is tiny, but we should bear in mind that if there's code that does a full bootstrap, prints or logs something small, then exits, then we are executing that code and taking memory for all modules defining constants regardless of whether they're used or not, and we can just not do that if the class method is otherwise good.

I don't think these are enough savings to justify doing this in themselves, but if people like the idea for other reasons, then it'd be nice to shave 50k+ of memory usage from heavily loaded sites, and especially from minimal callbacks like image derivative generation etc. which don't need the definitions of whether comments are threaded or flat loaded into memory.

Damien Tournoud’s picture

I always argued that using classes as namespaces just to autoload stuff in PHP is a really great idea. Crell hates the idea with passion, because it's not a pure way to use objects. I still don't get why.

catch’s picture

http://www.elxis.org/guides/developers-guides/php-defined-constants-vs-c... has a basic memory run down of 5,000 constants vs. 5,000 class variables. If you have to instantiate the class then you use a lot more memory, but on the assumption that you wouldn't most of the time, then there's potentially some savings.

catch’s picture

There's also

http://pecl.php.net/package/hidef

this lets you put constants in .ini files. If we found we were loading a lot of the lazy loaded classes, it could be a net loss to do this, so we might want to at least compare any such move with hidef (obviously we'd need our own method to load those constants if the extension was missing if it turned out to be interesting).

catch’s picture

Title: DX: put constants in classes » Optimize constant usage

Every time we load a module file, unless it does something weird, this is the only PHP code that we execute unconditionally (i.e. it's not inside a function or a class that gets called on demand).

http://shwup.blogspot.com/2010/04/about-constants.html has a good comparison of the current options, although they didn't supply the code to go with the numbers.

hidef looks like it might not be useful for Drupal - for example you might have trouble running different versions of the same module, or different versions of core on the same server - even if we namespaced constants via. PHP 5.3 namespacing that's still going to be tricky.

If we went with classes, the extra memory consumption is probably going to be outweighed by not loading all constants all the time (although we'll have our own memory overhead on top of this from caching the location of the class files for the registry). I'm removing it from the issue title, but that still feels like an option.

I want to look into apc_define_constants() - we might be able to replace that from Drupal when it's not available, but get the benefit when it's enabled.

#1058720: Lazy load modules would take some of the edge of this, but that's a long way off, feels like it depends on PHP 5.3 namespaces (or other major refactoring), and isn't mutually exclusive to doing something here.

Will have a look at doing my own benchmarks, at least define() vs. class constants vs. apc_define_constants().

catch’s picture

FileSize
4.5 KB

Here's a start on support for apc_define_constants().

This is by no means done, but I'm marking needs review for feedback since if the approach is OK, most of the rest would be converting modules and figuring out what to do with /includes. Also want to see if tests pass.

I realise I'm hijacking the issue a bit, but pretty much everyone who cares about this is already here and it's the same problem to fix. Justin if you want me to split this particular approach to another issue I'd be fine with that.

First a quick description of how apc_define_constants() works.

There are two functions:

apc_load_constants($key); - this will find constants stored in APC and load them, the constants are kept in APC between requests, which is how it manages to be faster than define().
apc_define_constants($key, $constants); this allows you to set some constants. Once you've set some, you can't add to the list, so you have to have specific groups.

Originally I was going to do this on a per module basis - but that would have meant calling apc_load_constants() and apc_define_constants() for each module in turn, which is unlikely to be much of a win.

However apc_load_constants() acts almost like cache_get() - you only need to know $key to call it, and if it returns TRUE, your constants are loaded and there's nothing else to do.

What the patch actually does, only modules are supported right now, and only user module implements the API.

Three new functions are defined, drupal_load_constants(), drupal_define_constants(), module_define_constants().

Also I've added hook_constants() - which returns array('CONSTANT_NAME' => 'value').

At the moment, I'm creating two groups of constants, "bootstrap" for bootstrap modules, and "enabled_modules" for all enabled modules. We could potentially just have a single module group, would need careful benchmarks in the non-APC case for cached pages but it would simplify the code a lot.

If APC is enabled, it will call drupal_load_constants($key), and if APC already has the constants, nothing else needs to be done.

If APC is a cache miss, it invokes hook_constants(), builds an array of constants, then defines them with apc_define_constants().

This means that when it's warm (which should be nearly all the time), there is no need to invoke hook_constants() or do anything like that - we're replacing maybe 1-400 calls to define() with about six function calls and and whatever APC does internally. Need to set up a test case for benchmarks to see what this looks like but it's well suited to Drupal I think. A major complaint against apc_get_constants() on php.net is that it's hard to use for frameworks where the constants would be split between lots of files, but hooks are ideally suited to this, so yay!

If APC is not enabled, then hook_constants() gets invoked each request, and we call define(). This is the tricky bit here - there is no benefit for sites that don't run APC at all, and there is a bit of overhead. I'm reasonably confident the overhead will be unmeasurable, but that will need to be verified.

One useful thing about this, it's completely backwards compatible - if you are accessing core constants you don't need to do anything, if you have a module that defines constants, you also don't need to do anything unless you want to. So it would be completely possible to backport this to Drupal 7 and no-one would notice (we may not want to do that, but it'd be an option to put it into Pressflow or maintain a patch - constants don't change that often).

The patch is entirely specific to APC. This is by design, since we are probably going to want to put bootstrap.inc's own constants into this API, which means it will need to be invoked even before DRUPAL_BOOTSTRAP_CONFIGURATION.

We could do something like Damien's old patch for variable_function() - define a couple of the lowest level constants, then this could be pluggable via settings.php. That might possibly be worth doing to allow a contrib project to implement support for hidef - as I write this I'm realising that might be doable via hook_constants() with some wrangling. Either way it makes sense to ship with an APC implementation in core, but we might want to leave it open in case something else comes along too.

The only other contender I've found for this is http://pecl.php.net/package/chdb - as far as I can see the end result of this is not real constants (nor class constants), so I think that's out of the running. Although it looks interesting otherwise.

catch’s picture

Checked another complex D6 client site, that has 498 calls to define() according to xdebug (including core constants), so 400-500 is not outlandish for the upper end of this, but I doubt any sites are going over 600 at least for D6.

So the two best cases for this probably:

Page caching - where we're loading bootstrap.inc constants - define() can look expensive in that context when there is hardly anything else going on.

Full bootstrap doing something other than rendering a full page (like a web services response) - that is the 500 constants case, where bootstrapping is the vast bulk of the request time.

If those two cases are both better with APC, that's great. If they're not measurably worse without APC, also great. Everything in between these two shouldn't be a regression.

sun’s picture

Looked at the patch, understand the code, but don't understand why we abstract PHP constants. Need to read up on the issue later. But that said, everyone will ask themselves "WTF?", too, so this definitely needs in-depth explanation + reasoning in phpDoc. I understand it's pre-alpha quality though.

neclimdul’s picture

Man, that's going to be hell on IDE's. Not that we're very friendly to them as it is but at least they can tell us a lot about well defined constants. my_module::MY_CONSTANT seems a lot better approach in that respect. Also for now I'm down with the (auto|lazy) loading even if we are cheating.

catch’s picture

Looking at hidef, unless I'm reading the README wrong (docs are pretty scarce), that also doesn't generate real constants, the examples use $CONSTANT instead.

Unless I missed something, that brings us down to apc_define_constants() vs. class constants + autoload.

@neclimdul, I don't use an IDE, but yeah losing proper doxygen/API module sucks a bit. Class constants aren't real constants, but presumably they'll still have good IDE support even if it's different.

At this point I think the best thing would be to compare class constants vs. apc_define_constants() with a few different scenarios. If the APC version isn't much better (or if it's worse), then class constants have it - since we don't need anything extra to make that work. However it'd be good to know what it looks like before we commit to something.

skwashd’s picture

I'm getting the sense that I'm missing something here. In the documentation for apc_define_constants() it recommends against its use:

[...] Since the main benefit of APC is to increase the performance of scripts/applications, this mechanism is provided to streamline the process of mass constant definition. However, this function does not perform as well as anticipated.

For a better-performing solution, try the » hidef extension from PECL.

Source: http://php.net/manual/en/function.apc-define-constants.php

I like the idea of Drupal being more OO and class constants fits with that approach. Once downside of class constants is that you can't use dynamic values when defining them, but you can for constants. For example:

// this is valid
define('MYMODULE_IMAGES_URL', drupal_get_path('module', 'mymodule') . '/images');

// this isn't
class MyModule {
  const IMAGES_URL = drupal_get_path('module', 'mymodule') . '/images';
}

I wouldn't consider this reason enough for not switching to lazy loading classes with constants, but something to consider in making the decision of how to proceed.

catch’s picture

It doesn't warn against its use, it just says it's not as fast as anticipated. APC is something we can support in core - there is no external dependency except checking whether the functions are available, which we already do in core (for example OpenID).

hidef has a documented limitation that the constants exist across the entire PHP installation (actually that might be an issue with the APC functions too, hmm), and also requires files to be written out (I think), along with php.ini configuration, this doesn't make it impossible to use with Drupal, but this is something we'd have to make constants pluggable for, not just a core-only solution. It might be cool, but it's a step further than either APC or class constants in terms of external dependencies.

Also I can't find any benchmarks published that compare the two and also present their methodology. http://shwup.blogspot.com/2010/04/about-constants.html has results but not methodology. So I want to get my own numbers to compare, which means a basic implementation.

My plan for this right at the moment:

1. Convert includes/bootstrap.inc constants to use drupal_define_constants()
2. get a comparison between define() and drupal_define_constants() when bootstrapping to DRUPAL_BOOTSTRAP_CONFIGURATION and DRUPAL_BOOTSTRAP_FULL and/or page cache hits.
3. Do the same for class constants (this is not a good use case for class constants since they will alway be loaded, but if we use them for modules, we might want to do bootstrap.inc for consistency, also we are comparing the techniques, not proposals for core patches at this point, and worst case when loading a module is that its constants will be loaded.
4. Create a dummy module (or more than one, or just convert core modules) with lots of constants that uses all three approaches - compare best and worst cases with that module enabled and the patch that's already here - that will be a fairer test of class constants.
4. Try to figure out hidef and see if I can get that to work just for bootstrap constants, so we can see how much faster/more memory efficient it really is compared to apc_define_constants().

In irc heyrocker pointed me to http://groups.drupal.org/node/140144, it is possible hidef or chdb might be viable (pluggable) storage backends for a settings system, so I'm hoping work here will help rule them in or out for possible candidates even if it's no use for the constants stuff.

catch’s picture

@neclimdul: what specifically will this break on IDEs? The constants are still defined as constants (with or without APC), so you should at least get proper autocomplete. I don't use an IDE so I don't know what other fancy stuff they can do.

I realised you can't use (at least some) dynamic variables with constants using the APC approach either - defining $_REQUEST time once per PHP process isn't going to be a good idea. So that's going to be a limitation with either approach.

One more issue we'll need to resolve with class constants is that drupal_parse_info_file() currently magically picks up defined constants that are used in the .info. I don't think we can put class constants syntax in .info files, so not sure what to do about that. Having said this, I'm not sure how much that feature is actually used, so we could also consider removing it.

neclimdul’s picture

@catch they're defined as constants at runtime, not in the source code. IDE's don't parse the runtime so they won't be parsed as constants.

Anonymous’s picture

Assigned: » Unassigned
catch’s picture

http://www.php.net/manual/en/language.constants.syntax.php

As of PHP 5.3 you can now define constants outside of classes with:

const CONSTANT = 'Hello World';

I am more or less given up on hidef and chdb for constants in core, only way I can think of doing it would be having module.constants.inc, using define() or const, then having core either automatically include the file, or making the constants loader pluggable (so you could skip loading the file and scan them instead), but it's a lot of infrastructure for what is a small gain compared to thing elsewhere.

However, I'm assuming const keyword will be a lot faster than define() (no function call, possibly optimizable by APC), will try to run basic numbers on this, would be a nice easy patch and free improvement if so.

This would not preclude moving some or all constants to class constants at a later date and similar changes.

Crell’s picture

If there is a performance boost from changing the way we define constants I am all for it. I still do not like moving them inside classes just for lazy-load purposes unless the class itself served some other purpose.

catch’s picture

Did some testing.

I generated two PHP files, each defines 50,000 constants then prints 'hello'; and nothing more. Normally in Drupal we define < 500 constants even with a tonne of modules enabled, but I'm mainly interested in ratio rather than absolute performance of this.

Then I hit them with xhprof in the browser.

define()

Total Incl. Wall Time (microsec):	274,038 microsecs
Total Incl. CPU (microsecs):	270,000 microsecs
Total Incl. MemUse (bytes):	9,832 bytes
Total Incl. PeakMemUse (bytes):	131,750,528 bytes
Number of Function Calls:	50,005
const

Total Incl. Wall Time (microsec):	69,979 microsecs
Total Incl. CPU (microsecs):	70,000 microsecs
Total Incl. MemUse (bytes):	8,344 bytes
Total Incl. PeakMemUse (bytes):	34,579,256 bytes
Number of Function Calls:	5

So overall, CPU and wall time reduced 3/4.

The actual performance saving here in real terms is going to be minimal, most likely somewhere around 0.5ms or 1ms at most. But it's a saving that comes for free so seems well worth doing.

Files are over 1mb so not uploading, but they look like this:

define('foo1', 1);
define('foo2', 2);
define('foo3', 3);

...
define('foo50000', 50000);
const foo1= 1;
const foo2 = 2;

const foo50000 = 50000;
sun’s picture

Status: Needs work » Needs review
FileSize
44.41 KB

Only difference is that const doesn't allow for expressions, so stuff like

define('DRUPAL_ROOT', getcwd());
define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);

needs to remain as define().

However, I'm fine with having exceptions to the rule here. This really comes for free, so let's leverage it.

---
Holy cow. That's a lot. ;)

define\('(.+?)',\s*(.+)\);
const \1 = \2;
sun’s picture

Issue tags: +Pressflow

btw, this is something that Pressflow could incorporate for D7 (if Pressflow still exists for D7).

catch’s picture

So here's the differences between const and define().

const is a language construct, the constants get defined at compile time, not run time.

Because it's done at compile time, it's much, much faster than using define() (which is a function call). My benches suggest five times faster, this stackoverflow answer suggests 10 times: http://stackoverflow.com/questions/2447791/define-vs-const/3193704#3193704

For the same reason, const only works with literals (string int, other constants), so we can't use it for something like REQUEST_TIME - those will need to still use define().

So the proposal would be a straight replace for define() vs. const syntax in core, document exceptions that will continue to have to use define(), and change the coding standards.

I suck at regexp, but fortunately sun doesn't.

Agreed this is something that would be worth having in Pressflow for D7 (there is a branch but it doesn't have any actual performance changes in it as far as I could tell reviewing the git log). Also contrib modules are free to require PHP 5.3 if they want to.

klausi’s picture

Issue tags: -Pressflow
FileSize
37.74 KB

Here is a sed regular expression I used to replace the obvious define() function calls:

find . -type f -exec sed -i -e "s/^define([\"']\([a-zA-Z0-9_]*\)[\"'], \([0-9]\+\|[\"'].\+[\"']\));/const \1 = \2;/g" {} \;

Matches calls to define() at the beginning of a line (the const language construct can't be used in if-conditions, functions, etc., I assume that intended define()s are such cases). Matches pure integer or string constants only (the const language construct can't be used for dynamically built constants).

klausi’s picture

Issue tags: +Pressflow

restoring tag

sun’s picture

um, no idea what's going here ;) #28 contains a full conversion already. It is not based on a regex; I stepped through all instances manually, and selectively converted lines that can be converted. (The regex I mentioned was merely used for the selective conversion and only for my own future reference, in case this needs a re-roll.)

klausi’s picture

Ah, sorry. I missed you previous comment. And I forgot to match empty string constants.

klausi’s picture

FileSize
45.03 KB

You missed some constants in scripts/run-tests.sh. This regex now also matches all your changes and fixes that:

find . -path './.git' -prune -o -type f -exec sed -i -e "s/^define([\"']\([a-zA-Z0-9_]*\)[\"'], \(-\?\(0x\)\?[0-9]\+\|[\"'].*[\"']\|NULL\|TRUE\|FALSE\));/const \1 = \2;/g" {} \;
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

jbrown’s picture

Love it!

xjm’s picture

Tagging.

Anonymous’s picture

nice work! looks all good to me.

neclimdul’s picture

Holy cow, is that peak memory usage number right? ~126M vs ~33M that's kinda a big deal too.

For the same reason, const only works with literals (string int, other constants), so we can't use it for something like REQUEST_TIME - those will need to still use define().

I can only assume this is because it doesn't know how to handle those at compile time. No biggie like sun said. Special case is reasonable here.
Edit: The above comment was because i misread catch's comment. please ignore.

Looks awesome. I'm curious how Komodo and Eclipse treat them(not at my dev computer) but we're using core PHP concepts with this approach so I don't think even if they didn't provide very good support now we should stop this improvement. Its entirely reasonable to assume they would be well supported in future releases.

catch’s picture

I think the peak memory is due to the way that xhprof measures memory, not the actual peak memory usage of the script. Memory usage from both xhprof and xdebug has to be taken with a very large helping of salt.

From xhprof docs:

Memory Profile
XHProf's memory profile mode helps track functions that allocate lots of memory.

It is worth clarifying that that XHProf doesn't strictly track each allocation/free operation. Rather it uses a more simplistic scheme. It tracks the increase/decrease in the amount of memory allocated to PHP between each function's entry and exit. It also tracks increase/decrease in the amount of peak memory allocated to PHP for each function.

Generally if you have a function that just does a cache get and puts something in a static (like drupal_get_schema()) it works pretty well. But if you have a small function called over and over again, it can get very wonky very quickly.

I'm not at the same machine where I have these scripts but if I remember I'll do memory_get_peak_usage() on them.

Crell’s picture

Nice work, catch and sun. I support this patch as well.

catch’s picture

Title: Optimize constant usage » Use const keyword to define classes instead of define()
Issue tags: +PHP 5.3

I added a brief issue summary, and I'm updating the title since this this is a straight conversion.

For anyone who's interested, there is a current PHP RFC for autoloading functions via SPL (on php-internals they suggested expanding this to constants). define() to const is a straight conversion, if we wanted to revisit lazy loading I think we should consider waiting to see what happens with that RFC.

catch’s picture

Title: Use const keyword to define classes instead of define() » Use const keyword to define constants instead of define()
klausi’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
44.23 KB

does not apply anymore. Reroll with script from #36.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

Assigned: Unassigned » Dries

I'd love to commit this but I was heavily involved in this issue and am going to try to avoid committing my own patches, so I'm going to put it into Dries' queue instead.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Personally, I think this is a bit of a WTF for new developers. I'd argue that it is unusual to use const's in this context.

I understand we did synthetic benchmarks with 50,000 constants. However, did we test this on a regular Drupal install? Any measurable difference in a real world situation?

I wouldn't want to commit this patch unless we can demonstrate an advantage in a regular setup.

sun’s picture

Status: Needs work » Needs review

this is a bit of a WTF for new developers

I think the actual WTF is PHP, and we're merely using the PHP language here.

So I agree that there is a WTF for new developers here, but not new developers of Drupal, but instead developers who are new to PHP 5.

I think it's perfectly fine to actively leverage the language constructs and features PHP natively provides to us, and as @catch already stated:

The actual performance saving here in real terms is going to be minimal, most likely somewhere around 0.5ms or 1ms at most. But it's a saving that comes for free so seems well worth doing.

catch’s picture

FileSize
140.83 KB

Here's what it looks like on a normal request to the front page. This is stock Drupal 8, minimal profile.

176 calls to define() took 407us wall time (so 0.4ms). I have seen real sites with about 500 calls to define() when they're loaded up with modules. Another request define() took around 600us (edit: us not ms) so there is obviously margin of error but I'd expect in the region of 350us to 1ms on most sites.

Total request took 45,111us for this run - so 45ms, meaning we could expect in the region of a 0.8% improvement for that request switching to const. It's not much, but it's also a saving on every single Drupal bootstrap since loading constants is directly in the critical path and above investigations showed there are no options for lazy loading (except class constants but that's going to be more unusual than just using const).

klausi’s picture

I'd argue that it is unusual to use const's in this context.

Why? The PHP manual clearly states: "You can define a constant by using the define()-function or by using the const keyword" http://www.php.net/manual/en/language.constants.syntax.php
Also the syntax is self explaining ("const" for constant).

neclimdul’s picture

Since define is still useful for dynamic constants, this feels conceptually equivalent to us using single quotes and double quotes in different places. As long as we're equally clear about the reason and when we use them this seems a reasonable improvement.

xjm’s picture

Status: Needs review » Needs work

This will need to be rerolled. Also, it's likely going to collide with the docs cleanup sprint (which is adjusting a lot of contstants' docblocks) so we might wait a week or so for the reroll.

klausi’s picture

Status: Needs work » Needs review
FileSize
45.68 KB

Rerolled with script from #36.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I think #50-53 cover Dries' concerns. Moving back to RTBC. @xjm I think Dries is mostly away for the next couple of weeks so docs patches are likely safe (and we're over thresholds at the moment).

Dries’s picture

I think it is my rusty C/Java background that threw me off here. After reading up on the php.net documentation it seems like 'const' is appropriate per sun's and other people's suggestion above. Thanks also for the benchmarking; that helped convince me.

I'm comfortable committing this, however the patch in #55 has a few fails so will need a re-roll.

vortex:drupal-head dries$ git apply ../f.p 
error: patch failed: core/includes/common.inc:42
error: core/includes/common.inc: patch does not apply
error: patch failed: core/modules/block/block.module:8
error: core/modules/block/block.module: patch does not apply
Dries’s picture

Assigned: Dries » Unassigned

I'm un-assigning this from myself as both catch and myself can commit this now a decision has been made.

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
45.81 KB

Here's a re-roll using the #36 script.

Anonymous’s picture

Status: Needs review » Needs work

after applying the patch, grep says we still have some stragglers:

core/includes/errors.inc:187: define('MAINTENANCE_MODE', 'error');
core/modules/syslog/syslog.module:9: define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
core/modules/syslog/syslog.module:12: define('DEFAULT_SYSLOG_FACILITY', LOG_USER);

catch’s picture

Status: Needs work » Needs review

Hmm, I looked through these and they might want to stay as define:

define('MAINTENANCE_MODE', 'error');

This is a conditionally defined constant in the middle of a function. It is more something I'd want to see wscci take care of (feels like maintenance mode is a 'context'). We could change that to const, but the whole thing feels messy there so I didn't want to touch it ;)

The other two are valid uses of define() because you apparently can't use const to define a constant to the value of another constant.

Moving back to CNR.

neclimdul’s picture

Yeah, maintenance mode as a context is my understanding of the plan for WSCCI. It mostly just a plan at this point but it makes perfect sense.

It is a bit weird though that all the other MAINTENANCE_MODE's are const and this one isn't but it does seem you can't conditionally set a const. Should we document it this one case since it is a little weird an inconsistent without other definitions of the same constant?

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

alllrighty then, RTBC. shame on grep for not actually being able to understand that code...

catch’s picture

Status: Reviewed & tested by the community » Needs work

No long applies due to node module docs patch..

klausi’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
46.14 KB

Reroll with script from #36. Back to RTBC as this is just a reroll.

catch’s picture

Title: Use const keyword to define constants instead of define() » Change notification for: Use const keyword to define constants instead of define()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Alright, committed/pushed to 8.x, thanks for the re-roll!

This needs either a change notification or a coding standards change or something.

xjm’s picture

Issue tags: +Needs change record

Tagging.

klausi’s picture

Component: base system » documentation
Status: Active » Needs review

So here is a first proposal for the coding standards change, current paragraph:

Constants

Constants should always be all-uppercase, with underscores to separate words. This includes pre-defined PHP constants like TRUE, FALSE, and NULL. Module-defined constant names should also be prefixed by an uppercase spelling of the module they are defined by.

Proposal:

Constants

Constants should always be all-uppercase, with underscores to separate words. This includes pre-defined PHP constants like TRUE, FALSE, and NULL. Module-defined constant names should also be prefixed by an uppercase spelling of the module they are defined by. As of Drupal 8 it is recommended to use the "const" PHP language keyword instead of define() for constants because of slightly better performance. Note that "const" does not work for dynamic constants or conditionally defined constants, you still have to use define() in such cases.

We could also copied that text straight to the change notification:

As of Drupal 8 it is recommended to use the "const" PHP language keyword instead of define() for constants because of slightly better performance. Note that "const" does not work for dynamic constants or conditionally defined constants, you still have to use define() in such cases.

xjm’s picture

Since the text is getting longish, let's use a bulleted list.

  • Constants should always be all-uppercase, with underscores to separate words. (This includes pre-defined PHP constants like TRUE, FALSE, and NULL.)
  • Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them.
  • In Drupal 8 and later, constants should be defined using the const PHP language keyword (instead of define()), because it is better for performance:
    /**
     * Indicates that the item should be removed at the next general cache wipe.
     */
     const CACHE_TEMPORARY = -1;
    
  • Note that const does not work for dynamic constants or conditionally defined constants. define() should be used in such cases:
    if (!defined('MAINTENANCE_MODE')) {
      define('MAINTENANCE_MODE', 'error');
    }
    

I added the change notice.

Edit: Added examples from core.

xjm’s picture

Issue tags: +Coding standards

Tagging.

xjm’s picture

Title: Change notification for: Use const keyword to define constants instead of define() » Update documentation: Use const keyword to define constants instead of define()
neclimdul’s picture

Can the note about dynamic constants(oxymoron anyone?) be nested or part of the third bullet point?

sun’s picture

"dynamic constants" is a wrong terminology. I'd phrase it like:

define() allows to assign expressions, while const does not.

Not sure whether there's a more appropriate way. A link to http://php.net/const won't hurt either, I guess.

xjm’s picture

Haha, "dynamic constant" is indeed... interesting.

How about:

  • Constants should always be all-uppercase, with underscores to separate words. (This includes pre-defined PHP constants like TRUE, FALSE, and NULL.)
  • Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them.
  • In Drupal 8 and later, constants should be defined using the const PHP language keyword (instead of define()), because it is better for performance:
    /**
     * Indicates that the item should be removed at the next general cache wipe.
     */
     const CACHE_TEMPORARY = -1;
    

    Note that const does not work with PHP expressions. define() should be used when defining a constant conditionally or with a non-literal value:

    if (!defined('MAINTENANCE_MODE')) {
      define('MAINTENANCE_MODE', 'error');
    }
    

Edited.

catch’s picture

That looks really good to me.

xjm’s picture

Edited my comment in #74 a few times. This is what I have for the tricky sentence now:

Note that const does not work with PHP expressions. define() should be used when defining a constant conditionally or with a non-literal value.

xjm’s picture

I updated the change notice to match #74.

aspilicious’s picture

BTMash’s picture

#74 looks excellent.

aspilicious’s picture

Title: Update documentation: Use const keyword to define constants instead of define() » Use const keyword to define constants instead of define()
Status: Needs review » Fixed

Sounds fixed

aspilicious’s picture

Title: Use const keyword to define constants instead of define() » Update documentation: Use const keyword to define constants instead of define()
Status: Fixed » Needs review

DAMNIT forgot about the docs... Srry for that.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

So this is actually rtbc

xjm’s picture

Title: Update documentation: Use const keyword to define constants instead of define() » Use const keyword to define constants instead of define()
Component: documentation » base system
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Alright, I've updated the documentation at: http://drupal.org/coding-standards#naming

We can refine the text further in a followup issue if needed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

klausi’s picture

Issue tags: +find and replace

Tagging.

Chi’s picture

I've just grepped the last D8 snapshot. We are still have a lot of define constructions.

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

nithinkolekar’s picture

Issue summary: View changes

is this also applicable for contrib module?

couldn't find any doc about what to do when converting d7 code to d8.

define('SOME_VARIABLE', 'value');

Close I found is #2807785: Move global constants from *.module files into interfaces