Story #78461
closed
VERY generically named constants in SystemEnvironmentBuilder should be removed or renamed
0%
Description
In SystemEnvironmentBuilder which is used by Bootstrap to process requests, we are currently defining a set of super-generic named constants which would be obvious candidates to cause a collision.
The constants are:
// A tabulator, a linefeed, a carriage return, a CR-LF combination
define('TAB', chr(9));
define('LF', chr(10));
define('CR', chr(13));
define('CRLF', CR . LF);
While it perhaps makes sense to define these (I don't necessarily agree it does; PHP_EOL exists for 95% of the use cases and authors can reasonably be expected to manually accommodate the last 5%) they certainly at the very least should be prefixed with a TYPO3-unique prefix.
The same goes for any other constants we may be defining that I've missed here. The main goal should be to get completely rid of these; keeping our constants to an absolute minimum and whenever possible, wrapping them in a class to prevent any possibility of tainting the global state.
This would be a breaking change but one we should do before the next LTS version - or we will have to live with them for a long, long time.
Updated by Benni Mack over 7 years ago
As a bugfix / workaround I would actually go with:
if (!defined('LF')) {
define(LF);
}
The PHP_EOL is a good replacement which we could add throughout the core in v8 then, and then put a note on it to remove the constants in v10 (although I consider them quite handy for my cases).
Updated by Nicole Cordes over 7 years ago
If we go to PHP_EOL we should have an eye on tests on Windows systems. This change might be very breaking then.
Updated by Simon Gilli almost 6 years ago
- Related to Story #68113: Base data package for installation of TYPO3 (database, possibly introduction package...) added
Updated by Claus Due almost 6 years ago
Just making a note here that the compromise of using "defined()" to create conditions before setting these constants: this will not address the actual problem that caused me to write this forge issue - that problem is that these constants are so generic that they are very likely to cause collisions. While I agree that it's fine to have conditions, adding those will not resolve this issue and must not close the issue. The renaming should still be considered!
Updated by Benni Mack almost 6 years ago
IMHO we can get rid of NUL, TAB and SUB directly. Going further one by one.
Updated by Benni Mack over 5 years ago
Hi Claus,
we're now down to these
defined('LF') ?: define('LF', chr(10));
defined('CR') ?: define('CR', chr(13));
defined('CRLF') ?: define('CRLF', CR . LF);which will be kept for v10. Can be tackled once we cleaned up deprecated code.
Updated by Christian Kuhn about 2 years ago
- Status changed from New to Closed
Hey. I hope it's ok to close this: We're left with CR, LF and CRLF. They're encapsulated in a "if defined" call, so they shouldn't clash with other frameworks, which if a different framework defines these, most likely defines them to the same values.
There is no huge benefit getting rid of the ones left, now. It only forces extensions to change their code without but does not add value. I see no huge issue in keeping the rest, at least for a longer while.