TYPO3 Core

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26899

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26899

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26899

Actually this would introduce something that does not comply to the definition of CSV.
See https://tools.ietf.org/html/rfc4180#section-2 for examples of CSV variants.

Comma Separated Values clearly states, that the values are separated with nothing more and nothing less but a character, which is usually a comma.
It does not say: Separated by character plus optional white space.

So IMHO we have to introduce a boolean to explicitely allow white space here and switch the behaviour of the method.
Default behaviours should stay as is though.

Who says this must comply to CSV??

Like Markus, I am not sure to understand the link with CSV...

Jo,

If I take the extension "gridelements" per example, I know that you use the function inList to check the "allowed" items in a backend_layout or in a backend_layout_gridelement.

The problem is if I put my list like this : allowed = text, textpic, header, image, menu, shortcut
Only the text will be take in account because of the whitespace.

And AFAIK, the whitespace in the TSconfig's lists are allowed anywhere else, no?
It is a bit strange for the interator if it is not working for a whitespace...

Thanks!

Here are some rmFromList alternatives: http://pastebin.com/Tc9vEKwk

The fastest 'non space' method I could find is the plain str_replace.

Here is an inList test: http://pastebin.com/FsCvbfnT

Hi Michiel,

What is your proposition to resolve this issue?

Thanks

Jo, Michiel, your suggestions on this topic please.

The methods inList() and rmFromList() work (as the method comments state) with comma separated lists.

As Joey points out, this means a list of items separated only with comma's.

These methods are setup like this because they are very fast.

You can find proper use of a comma separated list in many of the configuration arrays:

$TBE_MODULES['system'] = 'InstallInstall,BelogLog,ReportsTxreportsm1,txschedulerM1,dbint,config';

I would prefer those to be value indexed arrays, so we may even use a cheaper language construct: isset($TBE_MODULES['system']['BelogLog']) instead of this already expensive strpos() call, but that's not going to happen for version 6.2.

• TypoScript conditionmatcher
• Module loader
• Element browser recordlist
• Treeview
• Datahandler
• ExensionManagementUtility
• And so on and so on . . .

Changing the method as you suggest will slow down all 293 usages of this method. Doubling the amount of method calls.

Performance impact will range from about 0.2 - 1.2 % for a backend request (hit several regular pages like /backend.php, db_list, TS object browser etc.)

Also, you would need to change the rmFromList() to handle spaces as well.

People overwriting configuration arrays should know what they are doing and mimic the original value (i.e. not adding spaces).

Please leave those two methods at peace.

Eric Chavaillaz wrote:

Jo,

If I take the extension "gridelements" per example, I know that you use the function inList to check the "allowed" items in a backend_layout or in a backend_layout_gridelement.

The problem is if I put my list like this : allowed = text, textpic, header, image, menu, shortcut
Only the text will be take in account because of the whitespace.

On the one hand you are right, but on the other hand, the layout wizard inserts the values as a correct CSV list, without whitespace, which is what the integrator is supposed to do as well.

And AFAIK, the whitespace in the TSconfig's lists are allowed anywhere else, no?
It is a bit strange for the interator if it is not working for a whitespace...

The problem is, that you can not change the behaviour of this existing method without introducing possible breaking changes.
A list like the one you mentioned did not work before, but will be working after the change, which is why we should not touch the original method IMHO.

I understand there is a need to use spaces in some cases to improve readability. These are places where editors work with such values. In TypoScript or TSConfig. Those are also the places where it already is allowed and working as you note.

Changing settings in $GLOBALS['TYPO3_CONF_VARS'] is a step beyond what normal editors would do. Most of that code is more or less 'machine code'. It can be overridden though, but there be dragons as you also noted.

But my general point is that 99 more of such (more or less understandable) changes will make the core code about 100% slower by doubling the amount of method calls in often used methods.

I am working in the opposite direction by looking at the core code with a profiler and finding spots where we can actually reduce code complexity and method calls. You would be surprised how much faster the core has become by making many of such 'small' changes. Take a look at http://forge.typo3.org/issues/54517#note-8 for some numbers.

Added complexity slows down the code. Take a look at \TYPO3\CMS\Core\Utility\GeneralUtility::trimExplode() or \TYPO3\CMS\Extbase\Utility\ArrayUtility::arrayMergeRecursiveOverrule(). There are many methods in the core that very closely resemble native PHP methods but can not be deprecated because the TYPO3 behaviour is slightly different and the old methods may not be deprecated because of backwards compatibility. This is a pity.

Ok, so I think that the review must be abandonned and the issue rejected.

Thanks for the feedbacks.