Feb 11, 2015

Quality is no compromise

When it comes to the code quality, there are no compromises. You either write good code or bad code. There is nothing in the middle.

Many years ago I read an excellent book about code quality named “Clean Code”. That book goes to extreme and not all recipes are acceptable but I read and re-read, thought again and again until I set my own rules to writing code.

I think I have a certain advantage over people, who start using PHP as their first programming language. I started with Pascal and used assembler in parallel during my first five years of programming. Neither allows compromises. Pascal is very strict and assembler is just a razor sharp to errors: one minor error and your head is off. Next I used C++, which was more relaxed but than coded in Java for a couple of years, which was also very strict programming.

PHP is very relaxed when it comes to quality standards. You can assign a string to a variable and compare it to the number or to virtually anything else. You can assign any type to any variable, return anything from the function or do not return anything at all. If the programmer did not have good practice with other languages, he may start writing code, which is not that good. It is not necessarily true but it happens.

These things came to me today when I tried to use my TYPO3 extension named sentry. This extension automatically catches all kind of PHP warnings and exceptions (that you do not normally see) into a centralised repository when warnings and exceptions for various projects can be viewed, analysed and processed:




Details screen for the first warning looks like:



That extension worked fine with TYPO3 4.x and 6.x but did not work with 7.0. It appears that there is a new feature that handles exceptions in Frontend and bypasses normal TYPO3 exception handling or any other exception handling that the user may install globally.

I went to investigate and found this:


/**
 * Determine exception handler class name from global and content object configuration
 *
 * @param array $configuration
 * @return string|NULL
 */
protected function determineExceptionHandlerClassName($configuration) {
 $exceptionHandlerClassName = NULL;
 if (!isset($this->typoScriptFrontendController->config['config']['contentObjectExceptionHandler'])) {
  if (GeneralUtility::getApplicationContext()->isProduction()) {
   $exceptionHandlerClassName = '1';
  }
 } else {
  $exceptionHandlerClassName = $this->typoScriptFrontendController->config['config']['contentObjectExceptionHandler'];
 }

 if (isset($configuration['exceptionHandler'])) {
  $exceptionHandlerClassName = $configuration['exceptionHandler'];
 }

 if ($exceptionHandlerClassName === '1') {
  $exceptionHandlerClassName = 'TYPO3\\CMS\\Frontend\\ContentObject\\Exception\\ProductionExceptionHandler';
 }

 return $exceptionHandlerClassName;
}


I see two problems here that raise code quality alarm for me:

  1. The use of '1' (a magic constant). That is always a bad smell.
  2. The use of hard-coded class for the exception handler.
Somehow both these problems passed TYPO3 review process and got into the TYPO3 core. I am not sure how this could happen.

How could this be done better? Easy:
  1. Don't assign '1'. Check for empty() at the last if.
  2. Add configurable debug and production exception handlers to the configuration and choose appropriate handler using the debug option from the configuration.
I am getting more and more concerned about the code quality in TYPO3. Looks like with the old guys gone, TYPO3 gets more and more “so-so” code.

May be, this code was done with the idea to improve it later. But there is no such thing as "later" when it comes to code quality. It is either good or bad. There is no place for the bad code because it always causes problems.

I had to Xclass the ProductionExceptionHandler. Bad solution forced on me by the bad code in the TYPO3 core.

5 comments:

  1. Hey Dmitry. I could comment on many things you have written here, but I will only pick one thing to clarify:

    > I had to Xclass the ProductionExceptionHandler. Bad solution forced on me by the bad code in the TYPO3 core.

    If you would have read the documentation which is shipped with this feature[1] or had deeper look into the code, you would have seen that the hardcoded value you complain is actually only a default value. It can be changed by simply adding a global TypoScript option of your liking (disabling the feature or adding a different exception handler class).

    You can even influence the exception handling behavior for each content object rendering configuration[1]. This would not have been possible by adding a configuration option on top of the global, already too complicated (full of magic numbers a *user* needs to know and understand) and bloated error and exception handling configuration in TYPO3_CONF_VARS.

    Hope this info helps to improve the code in your extension, as you can now easily remove the XCLASS again.

    [1]https://git.typo3.org/Packages/TYPO3.CMS.git/blob_plain/HEAD:/typo3/sysext/core/Documentation/Changelog/7.0/Feature-47919-CatchContentRenderingExceptions.rst

    ReplyDelete
    Replies
    1. Hi Helmut!

      Yes, I read the documentation and explored the whole implementation carefully and attentively. However I still consider that it is not good enough.

      Firsts, it broke the existing behavior. In the past TYPO3 was famous for it compatibility. With this change, the old handler simply do not work. It seems natural to keep the existing behavior if the new handler is not configured. However it was not done. Breaking compatibility is bad.

      Secondly, it is inconsistent. All other error and exception handlers configure themselves in $TYPO3_CONF_VARS. May be, the author of the feature did not know that. Otherwise I cannot explain why he chose not to do it consistently.

      Thirdly, I am a big fan of "zero configuration" or "turn it on and it works" way. All my extensions behave this way: instal and it works. Yes, the notable exception is RealURL 1.x, which is not originally mine, but RealURL 2.x will have zero or nearly zero configuration too. By forcing the user to configure options in TypoScript for the exception hander, several problems are created:
      1. TypoScript is slower than PHP. A millisecond here, a millisecond there slows down the site by a couple of seconds.
      2. I know a project with about 12 separate instances in a single installation and same amount of configs. Imagine adding configuration there? No, includes are not allowed.
      3. If the extension with exception handler is uninstalled, one has to go and adjust TS everywhere as well. This is too much work to do!

      In other words, this feature breaks several important concepts: consistency, compatibility, simplicity, ease of use. I guess it was implemented quickly for author's own needs and than the author thought of submitting it to the core without thinking about generic usage. It is important to think about generic usage when submitting a privately developed feature. A private case is often not good enough for generic use and needs certa in rework.

      Next part of the comment will add some more information.

      Delete
    2. I also disagree that error and exception handling in $TYPO3_CONF_VARS is complicated and a *user* needs to know a lot.

      Firsts, we are talking about error handling! So it is developer's wok, not user's.

      Secondly, these options do not use magic constants and they are very well documented:

      'productionExceptionHandler' => \TYPO3\CMS\Core\Error\ProductionExceptionHandler::class, // String: Classname to handle exceptions that might happen in the TYPO3-code. Leave empty to disable exception handling. Default: "TYPO3\\CMS\\Core\\Error\\ProductionExceptionHandler". This exception handler displays a nice error message when something went wrong. The error message is logged to the configured logs. Note: The configured "productionExceptionHandler" is used if [SYS][displayErrors] is set to "0" or is set to "-1" and [SYS][devIPmask] doesn't match the user's IP.
      'debugExceptionHandler' => \TYPO3\CMS\Core\Error\DebugExceptionHandler::class, // String: Classname to handle exceptions that might happen in the TYPO3-code. Leave empty to disable exception handling. Default: "TYPO3\\CMS\\Core\\Error\\DebugExceptionHandler". This exception handler displays the complete stack trace of any encountered exception. The error message and the stack trace is logged to the configured logs. Note: The configured "debugExceptionHandler" is used if [SYS][displayErrors] is set to "1" or is set to "-1" or "2" and the [SYS][devIPmask] matches the user's IP.
      'errorHandler' => \TYPO3\CMS\Core\Error\ErrorHandler::class, // String: Classname to handle PHP errors. E.g.: TYPO3\CMS\Core\Error\ErrorHandler. This class displays and logs all errors that are registered as [SYS][errorHandlerErrors]. Leave empty to disable error handling. Errors can be logged to syslog (see: [SYS][systemLog]), to the installed developer log and to the "syslog" table. If an error is registered in [SYS][exceptionalErrors] it will be turned into an exception to be handled by the configured exceptionHandler.
      'errorHandlerErrors' => E_ALL & ~(E_STRICT | E_NOTICE | E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR), // Integer: The E_* constant that will be handled by the [SYS][errorHandler]. Not all PHP error types can be handled! Default is 30466 = E_ALL & ~(E_STRICT | E_NOTICE | E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR) (see [a href="http://php.net/manual/en/errorfunc.constants.php" target="_blank"]PHP documentation[/a]).
      'exceptionalErrors' => E_ALL & ~(E_STRICT | E_NOTICE | E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR | E_DEPRECATED | E_WARNING | E_USER_ERROR | E_USER_NOTICE | E_USER_WARNING), // Integer: The E_* constant that will be converted into an exception by the default [SYS][errorHandler]. Default is 20480 = E_ALL & ~(E_STRICT | E_NOTICE | E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR | E_DEPRECATED | E_WARNING | E_USER_ERROR | E_USER_NOTICE | E_USER_WARNING) (see [a href="http://php.net/manual/en/errorfunc.constants.php" target="_blank"]PHP documentation[/a]).

      The extension I wrote was several hours in work, thought first to take all possible scenarios into account. We fought unknown errors in about two weeks before I wrote it. After writing it, we found six errors in about 20 minutes and solved them with a hotfix for the customer's pleasure.

      Unfortunately, there is no way to achieve that with XCLASS if you take "install and it works" into account. You can tell me that I can use ext_typoscript_setup.txt but that option was deprecated even before TYPO3 4.5, so I would not like to use it. There is no other way, unfortunately.

      Delete
  2. Disclaimer: I'm the creator of the feature you are complaining about (I'm not sure whether you haven't seen or whether you just pretend to have not seen it).

    So you're making up 4 points here and I gladly comment any of those.

    1. Backwards Compatibility

    Yes, in production context the default behavior is changed. In development context it is not changed.

    Yes the application context is not widely known yet in our community and it interferes with the concept of development preset settings in the install tool. Both do more or less the same with a different approach. This needs streamlining but has nothing to do with this feature.

    Regarding the decision to turn exception handling on globally by default: I personally think that on a production site, an exception in a plugin should not cause the complete page to fail. The page should be rendered normally and an error message should be shown in the place the plugin normally outputs it's content.

    I think it is an improvement over the previous default behavior. But I agree that there may be different point of views on this. I wouldn't object to revert to the previous default behavior, if we find out that most people like it better that way for whatever reasons. It would be a online patch to change that.

    The good thing is: Everybody can turn this feature on or off very easily and tweak the behavior at a very fine grained level.

    2. Consistency

    As I tried to point out already: Making this feature configurable in TYPO3_CONF_VARS would be contradicting the initial goal, which is making this configurable on a *per content object level*. This means every plugin author can ship his or her own exception handling behavior of their liking for their plugins, not affecting the global configuration *at all*.

    I find it pretty consistent to make a feature, that affects the behavior of content object rendering, configurable in TypoScript, and not in TYPO3_CONF_VARS


    3. Strong defaults and flexibility

    As pointed out above, this feature just works by default with zero configuration. As extension developer you can easily ship your preferred behavior by using public (not deprecated) API like that in ext_localconf.php:

    // This would turn the feature off.
    \TYPO3\CMS\Core\Utility\ExtensionManagementUtility::addTypoScriptSetup('config.contentObjectExceptionHandler = 0');

    // This would globally use an exception handler class shipped with the extension
    \TYPO3\CMS\Core\Utility\ExtensionManagementUtility::addTypoScriptSetup('config.contentObjectExceptionHandler = Vendor\ExtensionName\COEH');


    Again, I gladly discuss which is a stronger default, having the content exception handling turned on or off.
    For everything else, I think I brought up stronger arguments and hopefully explained them in an understandable way.

    ReplyDelete
  3. Please don't get me wrong. I don't want to be right just for being right. I *really* want to understand your (or other peoples) concerns.

    So if anybody has a better naming for "config.contentObjectExceptionHandler" or has a better idea how to achieve the goal with a better configuration approach, I'm very happy to learn about it.

    There is no point in fighting each other, let's *collaborate* to deliver an outstanding product with 7LTS

    ReplyDelete