Is is_array check needed for the hook handling


(Tymoteusz Motylewski) #1

Do we need is_array check in hook handling?

In multiple places in the core, we’re checking for hooks like:

if (is_array($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'])) {
    foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'] as $_funcRef) {
           .....
    }
}

Which throws a PHP notice every time the code is executed and there is no hook registered.
Notices are bad because of the performance and they are indicating quality issues. The long term goal is to have notice free core, so we can encourage developers to also check for notices in their code.
There is an open review from Claus https://review.typo3.org/#/c/52416/ which uses 1A approach.

Solution 1

Do not check for is_array but rather with !empty/ isset or use ?? operator.
So the code will look like:
A)

foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'] ?? [] as $_funcRef) {
      .....
}

or
B)

if (!empty($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'])) {
    foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'] as $_funcRef) {
           .....
    }
}

Pro

  • Notice free code
  • Code is much cleaner (with A))
  • You will get a feedback (warning) when misconfiguring a hook

Con

  • It doesn’t check for is_array so if you misconfigure a hook (e.g. by setting a string instead of array) you’ll get a warning now.

Solution 2

First check for existence with e.g. !empty, and then for is_array

if (!empty($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'])
&& is_array($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'])) {
    foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue'] as $_funcRef) {
           .....
    }
}

Pro

  • we play safe, no change of behaviour, no warning when misconfiguring
  • no notices

Con

  • code is less readable, one condition more per hook

Organizational

Topic Initiator: Tymoteusz Motylewski
Topic Mentor: Tymoteusz Motylewski


(Fedir RYKHTIK) #2

IMHO I’m for Solution 2, as it’s more precise.

And will be great to have some core-level shortcut, something kind of:

hookProcess($hookArray, $hookItemProcessorCallback)

Where $hookArray = $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_befunc.php']['postProcessValue']

And $hookItemProcessorCallback contains the hook logic.


(Frans Saris) #3

I’m for A. The warning should be enough to tell the devs they misconfigured something. IMO that’s a plus as currently there is no feedback at all. Just a silent ignore of the misconfiguration.


(Riccardo De Contardi) #4

I don’t have a specific opinion on that, but I would prefer a warning in case of misconfiguration


(Claus Due) #5

This is actually on my list of PROs :wink:

Solution 2 is too noisy for my taste. My patch had the goal of reducing the code base size and complexity - not increasing it!

1B is redundant: the opcode produced by 1B is larger than that produced by 1A, and executing a foreach instruction on an empty array or iterator causes no execution. The same check is implied before the foreach iterates. Since it’s formally identical to 1A but both increases code size and complexity of resulting opcode the rational choice would be 1A. Which is why I created the patch like I did :wink:


(Claus Due) #6

I disagree, on both points. As commented, solution 2 is very noisy. Public API to call hooks just adds to complexity because we have no data transfer strategy and enumeration of hooks, but it would make sense if we had real enumerated hooks and could call the method with just the name of the hook and an (unrolled) list of arguments. Without that, it would simply be a wrapper around foreach() as you’d still need to know everything about the hook subscribers array and how to call each subscriber.

https://github.com/FluidTYPO3/flux/blob/development/Classes/Hooks/HookHandler.php#L115 may serve as inspiration. It has that public API that you can just fire and forget, and combines the purpose of signals and hooks. But that’s moving the discussion rather off topic.


(Fedir RYKHTIK) #7

Probably, it could be a nice internal feature for TYPO3 10 ?

Never too late to make TYPO3 better :slight_smile:


(Markus Klein) #8

I am in favour of Solution 1 A.


(Sebastian Michaelsen) #9

I’m following Claus here. 1A appears to be the best solution for me and getting a warning if you misconfigure a hook is not a Con.


(Stefan Neufeind) #10

I’m in for solution 1A as well. Make it noticefree, shortens code and is efficient. Having a separate hookProcess()-tool-function has some pros as well as cons, but I don’t think we’ll find an aggrement on that easily.


(Mathias Schreiber) #11

This is a nobrainer to me.
Solution 1A (@tmotylewski maybe name them 1-4 or a - d for easier reference ;-)) is clean, fast and its only drawback is that you get a warning if you fucked up.
Regarding the “breaking” part of the change:
Your system didn’t work in the first place (no hook got executed) so discussions here are pretty pointless in my eyes.
At least with that change we tell you :smiley:
In addition, I consider the amount of websites that just have broken config lying around somewhere pretty low and if they do, they deserve a warning.


(Sascha Rademacher) #12

I think 1A ist the Best Solution. For me a warning is a PRO.


(Helmut Hummel) #13

Go ahead with 1A
As pointed out, only has benefits for this use case


(Wouter Wolters) #14

I’m also in favor for 1A.


(Christian Kuhn) #15

From framework view I’d clearly opt to solution 2: This saves the framework from crashing if an extension does invalid stuff. Better safe than sorry from this point of view. I personally strive to always secure core code from extension code whenever possible. I typically throw exceptions in those cases. There are also places in the core that use excessive “isset+is_array” combinations: For instance the TcaMigration tries to make very sure it only does stuff if TCA is exactly as expected, and as early bootstrap code it is also important that it does not crash.

In this very case: We do have hooks where multiple classes can register and we also have hooks for single classes. This is always a hassle and using a hook usually means a developer has to carefully look where and how he registers. Getting a hook registration right is not always as easy as it should be. Not showing a warning if something is wrong, but “eating” and ignoring it then adds debugging overhead “why the heck is my hook not executed?”. An exception for misconfigured hooks would be perfect … but then we’re opening the ‘new hooks concept’ topic again, and this is not the question here.

So, I’d probably go with 1a in this case: it’s easy readable, it gives additional info to devs.

OT part: Striving for a notice free core is a huge task. Typically, a couple of hundred notices are already thrown by getIndPEnv() - a method that becomes like 100 times more ugly if you try to notice-free it (i tried that once). So the best direction here would be slowly getting rid of these calls (getIndPEnv(), GP(), …) and refactor more towards request/response and in the end deprecating these methods without putting energy into making these notice free. This would have an overall more beneficial effect on the codebase than just removing notices - This however does not mean we shouldn’t take care of notices. I’m trying to create as notice free code as possible when I write up new stuff or refactor bigger areas. All our younger code has significantly less number of notices (compared to code lines), I’d say.


(Jan Helke) #16

I am absolute in favor of 1A.


(Joerg Boesche) #17

I’m in for solution 1A as well.


(Benni Mack) #18

I also prefer solution 1A - I think you can just proceed and do that.


(Adrien Crivelli) #19

1A for me too. The new warning in case of misconfiguration definitely is a pro and code is easier to read.


(Alexander Opitz) #20

IMHO the core shouldn’t crash on false configuration and as lolly already wrote, getting TYPO3 notice free is a big task which can’t be solved easy.

So I would prefer to let it as is and replace it later with an other hook concept which is better working and removes more of this “mess duplicate code”.

So to save time in development I would vote for: “Do not change yet, fix with a new concept later on.”