Part of the Well Formed Errors Initiative

Problem/Motivation

"Be polite, Never Assert
Avoid the assert() mechanism, because it could turn a three-day debug fest into a ten minute one."
-- How to Write Unmaintainable Code, Roedy Green.

Even the experienced developers can be stumped or stalled when a large program they are working with throws a cryptic error or exception from an unfamiliar section of the code, especially when the actual failure is much further up the stack.

To prevent these sorts of headaches and make developing for Drupal 8 easier than ever before this issue ticket introduces the use of PHP's assert mechanism, optimizing the cache system as a case study and usage scenario. New traits are provided to allow unit tests to be written in an assertion aware manner so that the assert statements can be verified to fail when they should fail.

What are Assertions? What's the difference between Assertion, Exception and Error?

Assertions get confused with exceptions by novice and even some experienced programmers, so here is a summary of what they are, what they do, and what makes them distinct and useful.

  • Assertions document the code and openly state conditions that are impossible given error free code and module configuration.
    • Unlike errors and exceptions, assertions can be disabled, and in this state they have no more effect on the runtime than the comment text (so long as they are passed as strings to be evaluated, not as output from functions or expressions).
    • Because they can be disabled, assertions cannot be used as control structures.
    • Assertions are not a replacement for Test Driven Design - they are a supplement to it. PHP Unit Tests allow a block of code to be tested for its internal consistency, and more elaborate tests can be written to test interactions with other known blocks of code. Assertions can guard the code against mistakes in code and configuration files yet to be written.
  • Exceptions deal with user input mistakes, other programs malfunctioning, hardware failures, network outages, etc. If a situation can arise from outside the program, no matter how rare it is in practice, it should be handled by an Exception. Exceptions can be catch by code further up the stack using try/catch blocks. Neither assertions nor errors have this ability.
  • Errors either
    • Deal with situations which are by nature irresolvable (such as a parse error) and will require system shutdown - hence no opportunity should be given to catch them at all.
    • Log circumstances that may be important but do not leave the program in an unstable state - such as the calling of a function scheduled for deprecation.

Keep in mind the following: All bugs are errors, but not all errors are bugs. Assertions catch bugs -- errors in the program code and its developer configuration files (not the config files the final system admin writes themselves though). They are never to be used to catch errors which can arise from the user or environment as these aren't bugs (though failure to properly handle them is often called such).

Proposed resolution

The usefulness of the assert mechanism is demonstrated using the cache contexts system. During development it is necessary to validate both the cache tags and cache contexts since an invalid entry in either can invalidate the cache and lock up the site. During production new cache tags and cache contexts are not being created so such validation becomes redundant and expensive - profiling tests have shown a slowdown of the whole system of about 2% on just a basic page with around 50 cache tokens, nevermind a complicated view with hundreds or thousands of such tokens.

Assertions will replace calls to these verifications so that they only occur during development when assertions are turned on. Since PHP ships with assertions turned on by default assertions will be turned off using .htaccess.

Assert() behaves differently in PHP 7 than in PHP 5 - as with most errors in the system failures of assert() result in an exception throw in PHP 7. This patch will use the assert callback mechanism to force PHP 5.x to also throw AssertionException. This will allow unit tests to be written with the annotation @expectedException AssertionException and work whether the system is being tested under PHP 5.x or 7.

User interface changes

None.

API changes

No changes, minor additional options opened for developers writing tests.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because this is an enhancement and developer's aid only.
Issue priority Major
Prioritized changes Using assert has been verified to give performance gains in #2454643: Optimize cacheability bubbling (Cache::mergeTags(), ::mergeContexts(), BubbleableMetadata), so this minimal complexity version is being built for 8.0.x
Disruption Adding assertions will expose additional existing bugs, and already has during testing #2465749: [meta] Widespread HTML validation issue - The ID attribute MUST be unique on the page.. Priority will need to be assigned on those bugs. It is hoped that at least a few of these turn out to be the culprits behind the remaining critical bugbears. Note though the code itself isn't creating new bugs, simply exposing them.
CommentFileSizeAuthor
#331 interdiff-331--do-not-test.diff8.11 KBAki Tendo
#331 2408013-331.diff25.41 KBAki Tendo
#328 interdiff-327.diff5.93 KBAki Tendo
#328 2408013-327.diff25.42 KBAki Tendo
#325 2408013.325.patch25.25 KBalexpott
#325 320-325-interdiff.txt5.42 KBalexpott
#320 2408013-320.diff25.58 KBAki Tendo
#317 2408013-317.diff25.63 KBAki Tendo
#314 2408013-314.diff25.63 KBAki Tendo
#308 interdiff-308.txt936 bytesAki Tendo
#308 2408013-308.diff28.76 KBAki Tendo
#306 interdiff-306.txt8.39 KBAki Tendo
#306 2408013-306.diff152.33 KBAki Tendo
#304 interdiff-304.txt4.31 KBAki Tendo
#304 2408013-304.diff28.78 KBAki Tendo
#302 interdiff-302.txt5.47 KBAki Tendo
#302 2408013-302.diff29.06 KBAki Tendo
#299 2408013-299.diff28.92 KBAki Tendo
#297 interdiff-297.txt5.57 KBAki Tendo
#297 2408013-297.diff28.89 KBAki Tendo
#292 interdiff-292.txt37.55 KBAki Tendo
#292 2408013-292.diff29.47 KBAki Tendo
#284 interdiff-284.txt5.75 KBAki Tendo
#284 2408013-284.diff32.86 KBAki Tendo
#282 interdiff-281.txt1.35 KBAki Tendo
#282 2408013-281.diff33.26 KBAki Tendo
#276 interdif-257.txt3.93 KBAki Tendo
#276 2408013-276.diff33.29 KBAki Tendo
#274 2408013-274.diff31.38 KBAki Tendo
#272 interdiff-272.txt22.48 KBAki Tendo
#272 2408013-272.diff31.37 KBAki Tendo
#269 2408013-269.diff22.26 KBAki Tendo
#267 interdiff-267.txt27.1 KBAki Tendo
#267 2408013-267.diff17.02 KBAki Tendo
#263 2408013-262.diff21.98 KBAki Tendo
#258 interdiff-258.diff3.01 KBAki Tendo
#257 interdif-257.txt3.93 KBAki Tendo
#256 interdiff-256.txt3.68 KBAki Tendo
#256 2408013-256.diff23.64 KBAki Tendo
#255 interdiff-255.txt3.07 KBAki Tendo
#255 2408013-255.diff20.27 KBAki Tendo
#254 interdiff-254.txt1.66 KBAki Tendo
#254 2408013-254.diff20.84 KBAki Tendo
#252 interdiff.txt13.66 KBAki Tendo
#252 2408013-252.diff20.87 KBAki Tendo
#250 interdiff-250.txt6.5 KBAki Tendo
#250 Assert-Tools--2408013-250.diff20.21 KBAki Tendo
#248 Assert-Tools--2408013-248.diff19.59 KBAki Tendo
#247 Assert-Tools--2408013-247.diff19.6 KBAki Tendo
#244 Assert-Tools--2408013-244.diff19.64 KBAki Tendo
#242 Assert-Tools--2408013-242.diff19.63 KBAki Tendo
#239 Assert-Library-2408013-239.patch20.72 KBAki Tendo
#215 fault_system_assert-2408013-215.patch57.25 KBFabianx
#211 2408013-211.diff57.25 KBAki Tendo
#211 interdiff--2408013-211--do-not-test.diff1.82 KBAki Tendo
#209 interdiff--2408013-209--do-not-test.diff4.2 KBAki Tendo
#209 2408013-209.diff55.8 KBAki Tendo
#207 interdiff--2408013-207.txt3.87 KBAki Tendo
#207 2408013-207.diff54.88 KBAki Tendo
#205 Screen Shot 2015-05-01 at 9.20.18 AM.png57.25 KBAki Tendo
#205 style-changes--2408013-205.diff53.87 KBAki Tendo
#202 2408013-202.diff54.51 KBAki Tendo
#196 2408013-196.diff54.3 KBAki Tendo
#194 interdiff--2408013-194--do-not-test.diff20.64 KBAki Tendo
#194 2408013-194.diff54.28 KBAki Tendo
#185 2408013-185.diff57.14 KBAki Tendo
#185 interdiff--adjusted-tests--2408013--185--do-not-test.diff1.94 KBAki Tendo
#181 2408013-181.diff55.6 KBAki Tendo
#180 2408013-180.diff55.6 KBAki Tendo
#168 interdiff--extra-comments--2408013--168--do-not-test.diff2.21 KBAki Tendo
#168 extra-comments--2408013--168.diff40.99 KBAki Tendo
#162 wims-changes-assertions-on--2408013--162.diff40.04 KBAki Tendo
#158 interdiff--wims-changes-assertions-on--2408013--158--do-not-test.diff31.14 KBAki Tendo
#158 wims-changes-assertions-on--2408013--158.diff54.74 KBAki Tendo
#142 Screen Shot 2015-04-06 at 9.39.22 AM.png259.59 KBAki Tendo
#133 assertions-off-cache-test-240813-133.diff52.44 KBAki Tendo
#133 assertions-on-cache-test-240813-133.diff52.44 KBAki Tendo
#133 interdiff-2408013-133-131.txt1019 bytesAki Tendo
#130 assertions-on-cache-test-240813-130.diff51.77 KBAki Tendo
#130 assertions-off-cache-test-240813-130.diff51.77 KBAki Tendo
#130 faults-patch-240813-130.diff46.16 KBAki Tendo
#130 assertion-test-changes--do-not-test-240813-130.txt5.27 KBAki Tendo
#128 cache3.diff51.14 KBAki Tendo
#125 cache2.patch48.94 KBAki Tendo
#123 cache.patch46.07 KBAki Tendo
#121 fault-and-cache-assertions-2408013-121.patch46.5 KBAki Tendo
#119 fault-component-only-2408013-119.patch43.7 KBAki Tendo
#119 fault-and-cache-assertions-2408013-119.patch44.76 KBAki Tendo
#116 interdiff-2408013-115.txt2.43 KBAki Tendo
#116 2408013-115.patch46.16 KBAki Tendo
#114 removed-twig-symfony-2408013-114.patch48.53 KBAki Tendo
#114 cache-assert-optimize-2408013-114.patch1.06 KBAki Tendo
#101 interdiff.txt11.55 KBdawehner
#100 interdiff-2408013-100.txt1.51 KBAki Tendo
#100 2408013-100.diff24.92 KBAki Tendo
#98 interdiff-2408013-98.txt67.3 KBAki Tendo
#98 2408013-98.diff24.91 KBAki Tendo
#90 Screen Shot 2015-03-27 at 12.50.23 AM.png40.27 KBAki Tendo
#73 interdiff-2408013-73.txt24.8 KBAki Tendo
#73 2408013-73.diff45.12 KBAki Tendo
#71 2408013-71.diff45.13 KBAki Tendo
#68 2408013-67.diff81.08 KBAki Tendo
#64 2408013-64.diff68.52 KBAki Tendo
#64 interdiff-60-64.txt18.3 KBAki Tendo
#61 interdiff-60-61.diff13.85 KBAki Tendo
#61 2408013-61.diff64.07 KBAki Tendo
#60 interdiff.txt984 bytesAki Tendo
#60 2408013-60.diff50.22 KBAki Tendo
#59 2408013-59.diff46.98 KBAki Tendo
#53 Assertion_Handling-code_standard_compliant-2408013-53.diff29.79 KBAki Tendo
#44 interdiff-Assertion_Handling-ready_for_inclusion-2408013-44.txt20.96 KBAki Tendo
#44 Assertion_Handling-ready_for_inclusion-2408013-44.diff27.74 KBAki Tendo
#35 interdiff-Assertion_Handling-kernel_returns_assert_checked-2408013-35.txt6.91 KBAki Tendo
#35 Assertion_Handling-kernel_returns_assert_checked-2408013-35.diff26.99 KBAki Tendo
#29 Assertion_Handling-notice_fixes-2408013-29.diff22.09 KBAki Tendo
#25 interdiff-Assertion_Handling-2408013-25.txt305.59 KBAki Tendo
#25 Assertion_Handling-First_formal_patch-2408013-25.diff312.31 KBAki Tendo
#16 error_system_assertion_handler.diff9.79 KBAki Tendo
#7 patch.diff8.01 KBAki Tendo
#4 patch.diff25.11 KBAki Tendo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Aki Tendo’s picture

Note that this service would require PHP 5.4.8 to fully work. In prior versions of PHP (all the way back to 4) the assertion will be triggered, but the description code won't be passed to the service.

Aki Tendo’s picture

Wow. I had this mostly working in two hours, and fully up and going 2 hours after that (got hung up on multiline YML files. I need help composing a patch file, but this feature only requires a single additional class and one line be added to the settings.local.php file. It spits out error messages that look like the following..

http://www.screencast.com/t/LwBdXAIq5P

Aki Tendo’s picture

FileSize
25.11 KB

Attaching a patch meant more as a proof of concept than a working application. It adds assert statements to three spots in core. You can test and see the effects by deliberately making an invalid config file for a module and trying to install it, particularly an invalid TwigExtension.

The code does absolutely nothing if not turned on - to do so add this line to settings.local.php

Drupal\Core\Utility\AssertionRegistry::activateAssertionMode();

Feedback welcome.

Aki Tendo’s picture

Status: Active » Needs review
Aki Tendo’s picture

FileSize
8.01 KB

Ok, second try at making a patch, bear with me.

Aki Tendo’s picture

Incidentally, the assert statement for the moment is

assert(is_array($list), 'Core.Plugin.Discovery.YamlDiscovery.getDefinitions.listIsArray'
        .json_encode(['Module'=>$provider])
      );

The JSON encoding is optional, and to be used to display the values of variables that might be associated with the assertion failure. I don't know if PHP has a string limit on the message argument of assert. The label format is the namespace and function name of the event minus the lead Drupal namespace which would rapidly become redundant.

The system.assertions.yml file shows the format the errors will take when displayed.

neclimdul’s picture

Status: Needs work » Needs review

If you want testbot to run you'll want to put it in NR. It won't run tests on things in Needs Work.

Also as noted in IRC, you'll want to clean up your whitespace settings to remove tabs(only use spaces) and remove trailing whitespace.

Aki Tendo’s picture

I will do so either tonight or tomorrow while I tighten up its display. It did pass, but since it doesn't actually *do* anything until turned on that isn't surprising.

cosmicdreams’s picture

A question, and perhaps this is academic, but what is the benefit of using assert over using Exceptions to do exception handling?

JeroenT’s picture

Aki Tendo’s picture

@cosmicdreams
Assertions are not control structures of any sort. You can't catch them with a try catch block, if one gets raised and the ASSERT_BAIL flag was set then program execution will stop the moment the assert handling function concludes. They are a form of debug code that is meant to be turned off in production.

You use Exceptions to test for conditions which should be true.

You use asserts to test for conditions which MUST be true.

From Wikipedia -

Assertions are distinct from routine error-handling. Assertions document logically impossible situations and discover programming errors: if the impossible occurs, then something fundamental is clearly wrong with the program. This is distinct from error handling: most error conditions are possible, although some may be extremely unlikely to occur in practice. Using assertions as a general-purpose error handling mechanism is unwise: assertions do not allow for recovery from errors; an assertion failure will normally halt the program's execution abruptly. Assertions also do not display a user-friendly error message.

Or to put that another way, assertions can only fail if bad code or bad config files are introduced to the program in a development environment (If you monkey with configurations on a live server then you will be burned eventually) Once those are fully debugged and set up you port to production where assertions should be turned off, or at least set to warning in which case they behave much like trigger_error, noting an erroneous condition but not altering program flow.

The benefits of assert over exceptions
1. They go away entirely when turned off. Any conditional which monitors for an exception must be evaluated. Put enough of those in the program and there will be a performance hit. An assertion is no more significant to the PHP interpreter than comment text when turned off, and whatever it was doing is not performed when turned off.
2. They are fully independent of the try/catch logic of the program. You can't accidentally catch an assertion. In this way they are like errors, but unlike error evaluation assertions, again, can be disabled.

@JeroenT
Assertions aren't exceptions so whoops has nothing to do with them. That said, from reading whoop's docs, it looks like a poor man's assert. You can't turn them off in a live environment and you must steer around your try catch blocks to make use of it. The use case whoops purports to solve was solved by the assert statement in the language itself over 10 years ago in PHP 4. It's a classic case of a programmer who writes code blissfully unaware that better ways of solving the problem already exist.

An exception can always get catch by an overly greedy catch block somewhere in the code. That cannot happen to an assertion.

But again, they have their limits and they aren't a replacement for exceptions. When dealing with a condition that may fail because of API interactions or user misconduct exceptions are necessary. The very fact that assertions can be disabled prohibits them from being used as program logic code in any way. After all, exception try/catch blocks can be abused and used where normal control structures - if/then/else switch/case et al would be more practical. Assertions can't get abused in this way.

More on assertions here: http://en.wikipedia.org/wiki/Assertion_%28software_development%29

Aki Tendo’s picture

To further underscore the above let's look at one of the assertions I placed in the patch in the TwigExtension class

  public function getLink($text, $url) {
    assert($this->linkGenerator instanceof LinkGeneratorInterface, 'Core.Template.TwigExtension.getLink.noLinkGenerator');
    if (!$url instanceof Url) {
      $url = Url::fromUri($url);
    }
    return $this->linkGenerator->generate($text, $url);
  }

Whoever wrote this code originally didn't give a thought to the idea that there might not be a link generator. After all, if the services.yml files are all in order the service container will bind a linkGenerator to the TwigExtension object instance. This is also the sort of thing we don't need to check for with an exception - if the yml file is correct this assertion will always be true. Always.

Again, it is supposed to be impossible for linkGenerator to be null or something other than a link generator during normal program flow. However, when someone is still learning how to use the system and working with 6 months out of date documentation it's quite possible to get the config wrong. I did for about a day and a half. This assertion would have saved me that time and headache.

If the assertion is false the program will not continue - a fatal error will result when the object is called.

If we replace this assertion with an exception we gain nothing. We leave the program state flow here, and we cannot gracefully terminate. There's nowhere to go. Twig has been rendered unstable, which is why the AssertionRegistry doesn't use twig at all - not because I hate twig but because the library must stay as independent of the rest of the code as possible.

Placing an exception check here also confuses the issue. If I'm reading this code and I see an Exception throw then I consider the condition that causes the throw to be possible during normal program execution. If I see an assertion I know that the condition being asserted must be true.

Incidentally, Type hinting is a form of assertion. Like assertions, the program unceremoniously ends if a type hint is violated, and such violations do not occur in normal program flow. Unlike assertions they are always checked for though, making them little better than trigger_error in the way Drupal / Symfony uses them (most of the whole service container circus could be avoided by properly using the Reflection API to provide Dependency Injection, and therein is where their true power lies).

That's about all I can say on this.

Aki Tendo’s picture

Title: Drupal badly, badly needs to start using the PHP assert mechanism. Proposal: AssertionService » Assertion Handling
Issue summary: View changes

Changing to a cleaner title.

Aki Tendo’s picture

Aki Tendo’s picture

I've retracted the scope on this back to the two classes that must be added and a two line change to index.php that allows them to be loaded. Due to the nature of the patch I'm confident that it will pass automated testing, but manual testing is needed. I'm not even sure PHPUnit can accurately deal with classes that work with assertion handling because of the potential conflicts with PHPUnit's own assertion handlers.

Again, if your kneejerk reaction to this is "why not use exceptions?" then read this THOROUGHLY...

http://en.wikipedia.org/wiki/Assertion_%28software_development%29

before commenting. Assertions are NOT a replacement for exceptions -- see the discussion above for even more details.

Aki Tendo’s picture

Status: Needs review » Active
tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Error/AssertionHandle.php
@@ -0,0 +1,106 @@
+class AssertionHandle extends Error {

This whole file needs to follow Drupal's coding standards.

I think we need to use it in at least one place, like the TwigExtension example from #7.

Aki Tendo’s picture

I'm still trying to learn those standards, bear with me. Years of VIM style code blocking doesn't go away overnight. The patch name itself also needs to follow standards.

mpdonadio’s picture

Status: Active » Needs work

Out of curiosity, with your misconfigured service, did the core unit test suite pass? If they didn't, did they give any indication as to what was broken?

Few side notes, https://dreditor.org/ will give you patchname suggestions, taking the issue title and comment number into account.

PhpStorm is pretty invaluable for D8 development, and you can configure it to confirm to Drupal coding standards and format code for you. This really helped me break my K&R habit.

Aki Tendo’s picture

At the time of the hunt I didn't know if they were there. I guess I could put the module in question into the same broken state and try it out to see if they'll catch it.

In other news for this project, I've been rewriting how the system finds YAML files for itself on its own. I can't rely on YAML discovery because assertions could be thrown before the service container loads, and also they can be thrown from core code, not just module code. What I plan to do is have the assertions be part of an errors.yml file and while this module isn't going to have anything for exceptions or errors it will be using the conventions planned for the more global errors system in 8.1.

The handlers will make the assumption that the errors file is either in the same directory as the file the assertion filed from or one of its parents, so it traverses up to the root trying to find it and when it does it loads. Here's the code at the moment...

  protected static function loadErrorExplanations($backtrace) {
    $path = $backtrace[1]['file'];
    $root = dirname(array_pop($backtrace)['file']);

    while ($path !== $root) {
      $path = dirname($path);

      if (file_exists($path . '/errors.yml')) {
        try {
          $parser = new Parser();
          return $parser->parse(file_get_contents($path . '/errors.yml'));
        }
        catch ( ParseException $e ) {}
      }
    }

    return null;
  }

The trace in question is ran from the assertion handler itself, so element 0 is the assertHandling function, element 1 the assert statement. The last element will be the index.php file and since it's code is the only thing running outside of a global namespace it will need special handling code to be written if an assertion needs to be there (at the moment I see no reason to place an assertion in index.php). The directory of index gives us the site root - remember again this handler cannot know whether DRUPAL_ROOT has been defined yet, so it needs a consistent method find site root. I don't know if the method above will work with drush since I don't know drush's point of entry yet - but worst case scenario drush simply leaves assertions turned off.

The key that is returned is the namespace and class prefix for the error message. Here's an example of how it will work from the DrupalKernel class. First the assertion itself:

assert(file_exists($this->sitePath), 'noSitePath');

Curiously, the DrupalKernel has a conditional that tests for an unset sitePath, but as I've pointed out before that's not what Exceptions are for - there should be no possible way in a bug free codebase and configuration for that to happen, so this check should be left to assertions (and if I'm wrong and it is possible I'll need to be corrected. In any event when I commit the next patch I fully expect this correction to break any test checking for that exception to work, but the tests will need to be amended.

Anyway, there is no errors.yml in the directory where DrupalKernel resides nor are there plans for one, so the handler will go up until it finds the nearest one is at /core/lib/Drupal/errors.yml This will make the key "Drupal.Core.DrupalKernel" and the full name of this assertion "Drupal.Core.DrupalKernel.boot.noSitePath" The Yaml file reads

assertions:
  Drupal.Core.DrupalKernel.boot.noSitePath: The site file path has been left empty, does not exist, or PHP cannot read the file due to permissions misconfiguration.

This message is the "core" message and will be the first one displayed. Any module in the system can add messages by placing the path above in their errors.yml file, but those extra messages can only load if the service container was successfully loaded.

Modules find their errors.yml file the same way - this in the very unlikely event the module author finds a way to flub up the service container at run time.

I think this schema has the least room for error. The assertion key is just a namespace / classname / function / assertionname so it's straightforward.

Thoughts?

Aki Tendo’s picture

Not quite ready to release the next patch, but I have a screen capture of the thing in action.

http://screencast.com/t/ih41Wi6YCd

pfrenssen’s picture

A half patch is better than no patch ;)

Aki Tendo’s picture

Ok, Here it is, the first formal patch - I've read all the instructions I've been given and gotten this thing working reasonably well though I'm still a bit mystified on how it's going to be auto-tested given it's nature.

All previous patches should be ignored since they were made while my understanding of formatting and patch creation conventions where incomplete.

There is an excellent chance this will fail automated testing because I removed an Exception in favor of using an assertion in DrupalKernel::boot. The Exception in question and it's attendant conditional are being used for the precise purpose assertions are intended for.

The Assertion handler lives in the Drupal/Core/Error The larger project it is part of will eventually replace all other error handling code in the system including the Drupal/Core/Utility/Error class. Edits to other classes are with the intent of making them take advantage of the handler's presence. The assertions that will accompany this patch are those within Drupal Core, particularly during the boot phase. Other assertions, such as those for twig, will have their own issue tickets. Same with errors and exception updates when the 8.1.x version begins.

There are 4 classes currently
AssertionHandler - handles assertion failures
BaseHandler - abstract base class for error and exception handling of any type.
ErrorSetup - sets up error handling
RenderHTML - Renders the error response.

I'll need a response renderers for CLI (drush) and possibly javascript.

Note - I made an interdiff, comparing the patch against 8.0.x, ignoring previous patches on this issue which aren't properly submitted.

Aki Tendo’s picture

Status: Needs work » Needs review
mpdonadio’s picture

+++ b/core/composer.json
@@ -16,6 +16,7 @@
+    "symfony/var-dumper": "2.6.*",
     "symfony/yaml": "2.6.*",

Doesn't Dries need to approve all new libraries, or is there an exception for additional Symfony components?

Aki Tendo’s picture

I dunno. Anyway I removed the dependency and changed the code such that if the dump function is defined by symfony vardump it will call it, otherwise there will be no stack dump (too great a risk of crashing the browser what with how drupal nests objects)

New patch. No interdiff because it would be useless to try to read it - pulling var dump just makes the interdiff harder to read than the diff file.

Aki Tendo’s picture

Status: Needs work » Needs review
Aki Tendo’s picture

Question - I'm currently going through DrupalKernel and writing assertions to enforce the phpDoc's declared return types. For example for this phpdoc statement proceeding DrupalKernel->attachSynthetic

  /**
   * Attach synthetic values on to kernel.
   *
   * @param ContainerInterface $container
   *   Container object
   *
   * @return ContainerInterface
   */

I have added immediately proceeding the return statement..

assert($container instanceof ContainerInterface, 'invalidReturn');

I'll be doing this somewhat blindly, but if this raises an error after verifying the assertion is indeed written correctly should I submit that as a separate bug?

For it occurs to me, if I'm writing a new bug handling piece to the suite, I just might catch a bug or three.

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Error/RenderHTML.php
@@ -0,0 +1,32 @@
+    require (__DIR__ . '/templates/default_html.phtml');
...
diff --git a/core/lib/Drupal/Core/Error/templates/default_html.phtml b/core/lib/Drupal/Core/Error/templates/default_html.phtml

@Aki Tendo Silly question, why is there a phtml extension file here? We don't have any of those in core, it's normally just a .php file.

There are a number of coding standard issues still in this patch, we can point them out if that is helpful to you?
They live here: https://drupal.org/coding-standards

And the prod/dev thing could have some debate around that as it was discussed before and may be related to #1537198: Add a Production/Development Toggle To Core

joelpittet’s picture

@Aki Tendo the Issue summary is great by the way, this sounds like a very interesting idea! Color me interested:)

pfrenssen’s picture

Thanks for reworking the patch without depending on additional libraries, this will make it more likely to be accepted. This looks very interesting.

Aki Tendo’s picture

This patch removes the unknown environment code notice - that's a can of worms for another day. I went through Drupal Kernel and added assertions equivalent to the PHP Doc's declared return types when the function is returning an object (one exception where the function is returning a value from another function in DrupalKernel, so the second assertion would be redundant).

The next step is to clean the coding issues (Yes, please help, I've read the coding standards multiple times now and I keep finding exceptions and fixing them as I go as I'm getting used to it), and also change the extension from phtml to php on the one render template this system is using. Finally I'm going to traverse the functions in the include folder and assert that their returns match what PHP docs says they should.

Note that if this patch doesn't pass testing (it did test locally, I was shown how to get that up and running thankfully) then either the assertion is wrong or a new issue has been uncovered. Assertions are honored by PHPUnit and seem to also get honored in Simpletest - I had them go off a number of types when I entered bad ones in during this last build round. That gives me even more incentive to get this working since no code changes will be needed to have this work picked up by the unit testing software.

Aki Tendo’s picture

Hiding older patches.

pfrenssen’s picture

+++ b/.htaccess
@@ -24,6 +24,10 @@ ErrorDocument 404 /index.php
+# Uncomment to activate assertions and their handlers. This is only needed
+# when developing new code, do not turn this on in production.
+# SetEnv DRUPAL_ERRORS_ASSERT_ACTIVE true

I see you are initiating this by setting an environment variable. All other settings that are intended for development environments are in the file sites/default/settings.local.php. It would be very convenient to use this as well, so that all development option live in the same location. Is there a reason to use environment variables instead? Perhaps the reading of the configuration happens too late?

Aki Tendo’s picture

Perhaps the reading of the configuration happens too late?

That is exactly the reason. I want the error system to be ready an able to function from the very, very beginning of system start to maximize its ability to aid development - and after checking the kernel there's actually a sizable window for things to go wrong before the settings load. Some 12 assertions are checked before the Settings file is read.

As far as keeping such settings local, the best way to do it would be to move these SetEnv directives into the dev server's httpd.conf file - this way there is no chance of accidentally deploying the settings.

I have considered exposing a method to change the Error Environment out in settings.php - while redundant it would provide a more familiar configuration method.

Do note that the way I'm reading the values in bypasses certain Apache security modules that would otherwise render the variables unreadable. I'm actually running my sandbox under those settings.

Aki Tendo’s picture

Issue summary: View changes
Aki Tendo’s picture

Issue summary: View changes

Over the last several days of development the goalposts have shifted around and the process of actually writing and managing error messages has opened my eyes up to flaws in the approach that will be addressed in what is hoped to be the final revision before locking down this module. The writeup of the module is also being revised to reflect these changes.

Aki Tendo’s picture

Title: Assertion Handling » Add assertion handling, and add assertions to the DrupalKernel class based on its existing phpdoc comments..
Issue summary: View changes

Title change, typos.

Aki Tendo’s picture

Assigned: Unassigned » Aki Tendo
Category: Feature request » Task

After reading the beta changes guide, particularly the difference between a task and a feature request, I've come to the conclusion I've mis-categorized the project and adjusted it accordingly. https://www.drupal.org/core/beta-changes

This isn't a 'feature' that end users ever see - its a tool for developers that can be entirely turned off, so its creation is a task.

Aki Tendo’s picture

Issue summary: View changes

Applying proper formatting - https://www.drupal.org/issue-summaries

Aki Tendo’s picture

This patch, should it pass automated testing, should be ready for inclusion.

benjy’s picture

  1. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +      parent::parseMessage( $message );
    

    Extra white spaces around $message. Actually, this seems to be throughout the patch.

  2. +++ b/core/lib/Drupal/Core/Error/BaseHandler.php
    @@ -0,0 +1,294 @@
    +   * Error definitions.
    +   * @var array
    

    Missing new line between on lots of the class parameters.

In general it would be good to do a once over the patch for the two issues above, also checkout the Drupal coding standards. https://www.drupal.org/coding-standards

cilefen’s picture

@Aki Tendo There is a Drupal standards checker for the command line. See https://www.drupal.org/node/1419988

Aki Tendo’s picture

I installed the code sniffers indicated and got them running, then found they where changing the code contrary to the stated guidelines on the coding standards page, even in Drupal mode.

At this point I can only conclude you folk are hazing me. I'm not interested in playing silly college student games, and if that's a requirement to contribute to your project I'm no longer interested as I've got better things to do with my time. While I understand a need for code format consistency, there is such a thing as taking it way, way too far, and this is certainly it. Good day.

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -796,6 +805,11 @@ public static function bootEnvironment() {
    +    /*
    

    Should be /**

  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -1112,7 +1130,9 @@ protected function dumpDrupalContainer(ContainerBuilder $container, $baseClass)
    +    $interface = $this->container->get('http_kernel');
    ...
    +    return $interface;
    

    would http_kernel be a better variable name?

  3. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +  /*
    

    needs a new line between { and /, and should be /**

  4. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +  /**
    

    should be a blank line before here.

  5. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +   * Assert Callback
    

    Should have a full stop

  6. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +   * @param string filename failure occured in.
    

    Our standards is

    * @param {type} ${variable_name}
    *   {description}
    <code>
    </li>
    
    <li>
    <code>
    +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +    if ( static::$messageCode ) {
    

    our standard is

    if (static::$messageCode) {
    
  7. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +   * Overrides parse message to check if we are beneath 5.4.8, if so we won't
    

    Our standard is for a one-line <80 char short summary. Subsequent lines go after summary. e.g.

    /**
     * Overrides parse message on specific php versions.
     *
     * {rest of message here}
     */
    
  8. +++ b/core/lib/Drupal/Core/Error/AssertionHandler.php
    @@ -0,0 +1,51 @@
    +  protected static function parseMessage( $message ) {
    +    if ( $message === '' && version_compare(PHP_VERSION, '5.4.8') === -1 ) {
    ...
    +      parent::parseMessage( $message );
    

    no need for spaces inside parenthesis

  9. +++ b/core/lib/Drupal/Core/Error/BaseHandler.php
    @@ -0,0 +1,294 @@
    +   * @var array
    ...
    +   * Error definitions.
    ...
    +   * Error Environment variables set using the SetEnv directive in .htaccess
    ...
    +   * Root path relative to this file - we can't rely on DRUPAL_ROOT being defined.
    ...
    +   * Path to the error definitions file
    ...
    +   * Message string provided by the error or assert statement, Exception object
    

    our standard has a blank line between description and @var

You can use the automated tool for just the new files, I'd ignore coding standards in existing files that aren't pointed out here.

cilefen’s picture

At this point I can only conclude you folk are hazing me. I'm not interested in playing silly college student games, and if that's a requirement to contribute to your project I'm no longer interested as I've got better things to do with my time. While I understand a need for code format consistency, there is such a thing as taking it way, way too far, and this is certainly it. Good day.

I hope you are just kidding about that. Nothing could be further from the truth. I take this stuff seriously, I mentor new contributors, and I do this for a living. I assure you your contributions to Drupal are much-needed.

cilefen’s picture

The code sniffer standards are real, and they work. They are not foolproof. You can ping me any time on IRC and I will step you through setting up and integrating it with your editor if that is preferable. And I will answer any questions you have about the standards.

But it is a requirement that new code being contributed to Drupal meets the standards.

pfrenssen’s picture

At this point I can only conclude you folk are hazing me. I'm not interested in playing silly college student games, and if that's a requirement to contribute to your project I'm no longer interested as I've got better things to do with my time. While I understand a need for code format consistency, there is such a thing as taking it way, way too far, and this is certainly it. Good day.

I hope you realize these people you are attacking are spending their free time trying to help you get this patch up to the required standards.

cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Tagging Novice because the code style fixes in #45 and #48 can be handled by a novice. Note, this does not mean the issue is Novice, just this aspect.

Aki Tendo’s picture

I have mild dyslexia. So spacing and ordering is already frustrating enough and I typed that off after getting frustrated at the code sniffer reporting back this as an error:

if( condition )

I had misread the standard - it's referring to having the space after the statement but before the bracket.

if (condition)

Either that, or I mis-memorized it.

In either event I reached the limit of my frustration for one night and typed off an angry response to that frustration. I'm sorry.

This morning with rest and fresh eyes I was able to figure these problems out. The patch attached is able to pass the code sniffer except for DrupalKernel - and none of my lines are tripping errors.

An interdiff would be useless to provide, there's just too many false positives scattered through the files.

Aki Tendo’s picture

Status: Needs work » Needs review
kerby70’s picture

I immediately receive a 500 error when I uncomment 'SetEnv DRUPAL_ERRORS_ASSERT_ACTIVE true' because my config did not allow for environment variables to be set at the .htaccess level. I was able to set DRUPAL_ERRORS_ASSERT_ACTIVE in my vhost. I think further discussion is warranted on the topic of environment variables.

I would suspect removing this error check needs more familiar experienced assessment than I can give.

		@@ -385,6 +392,7 @@ public function getAppRoot() {
    * {@inheritdoc}
    */
   public function boot() {
+    assert('file_exists($this->sitePath)', 'noSitePathSet');
     if ($this->booted) {
       return $this;
     }
@@ -392,10 +400,6 @@ public function boot() {
     // Start a page timer:
     Timer::start('page');
 
-    // Ensure that findSitePath is set.
-    if (!$this->sitePath) {
-      throw new \Exception('Kernel does not have site path set before calling boot()');
-    }
     // Initialize the container.
     $this->initializeContainer();

I believe manual testing would be required but I am not able to define the test cases that would or would not cause these assertions to trigger.

Aki Tendo’s picture

Code passes testing with that exception removed, which means (chillingly to me) the tests aren't trying to cause a failure and muck things up. The exception reads like an impossibility if all is setup right.

As for environment variables - Zend Framework uses them extensively, but they have issues as you've observed. My personal thoughts is they are ok for an error system, and further having the error system permit the settings.php file to change settings would be useful for those who have environments where they don't work. Environment variables lets assertions and errors thrown during the window between startup and settings load work - admittedly that's a somewhat small window.

Another reason they work for the errors system is that if we assume reasonable production defaults (no assertions, non-verbose mode, don't stop except for warns or higher) the configurations where they don't work should be a non-issue since developers should be able to adjust their local environments easily. Production, not so much, but none of the features of this package are meant for production. The full errors system should keep this in mind when the time comes.

Aki Tendo’s picture

A better way than Set_Env occurred to me - just clue off of php_value assert.active. If asserts are on, turn the handler on. Even if .htaccess isn't an option the setting can be defined in PHP.ini

This is simple and clean and I wonder why it didn't occur to me before.

Aki Tendo’s picture

Issue summary: View changes
Aki Tendo’s picture

Title: Add assertion handling, and add assertions to the DrupalKernel class based on its existing phpdoc comments.. » Fault System - for 8.0.x Assertions only, For 8.1.x this will will be a complete error handling system rewrite.
Issue summary: View changes
Issue tags: -Novice
Parent issue: » #2451793: [META] Assert Statement Use in Drupal
Related issues: +#2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts)
FileSize
46.98 KB

With this patch, we start over. For the last time. I promise.

The goalposts have been moved back to something sane - that means no changes to DrupalKernel. This is just the Fault System and it's tests, which is quite enough for one ticket to handle. There is currently one child handling the template assertions. The changes to DrupalKernel (adding assertions) will be part of a separate ticket.

The fault system now figures out what mode it should be in based on the the php.ini settings for assertions and errors.

Aki Tendo’s picture

FileSize
50.22 KB
984 bytes

Bugfix in the AssertionTestTrait - during testing it wasn't performing file lookups to see if an error message has been created yet. The reason this isn't a test fail is having said message is a dev convenience, not a requirement for the code to run.

Aki Tendo’s picture

FileSize
64.07 KB
13.85 KB

Optimizations to the fault system, also to the kernel so some things might break (essentially, for performance reasons if you can set something in .htaccess or php.ini you *should* do so).

Aki Tendo’s picture

FileSize
18.3 KB
68.52 KB

Note to self - rerun all tests before making a patch :(

Aki Tendo’s picture

Status: Needs work » Needs review
Aki Tendo’s picture

Rolling back to last known good.

Aki Tendo’s picture

FileSize
81.08 KB

With the one should be safe optimization.

Aki Tendo’s picture

Status: Needs work » Needs review
dawehner’s picture

Let's first post a easier reviewable patch.

  1. diff --git a/.htaccess b/.htaccess
    index fd7bd29..0e86304 100644
    --- a/.htaccess
    +++ b/.htaccess
    @@ -39,6 +39,13 @@ AddEncoding gzip svgz
       php_value mbstring.http_input             pass
       php_value mbstring.http_output            pass
       php_flag mbstring.encoding_translation    off
    +
    +  # Assertions.
    +  # By default PHP has these turned on. Working from the assumption this is
    +  # production, turn them off. Change the string to off and uncomment if you
    +  # want them on but your environment defaults to off for any reason.  If you
    +  # enable assertions, you will also enable the ErrorSystem.
    +  #php_value assert.active                   off
    

    It would be nice to have it per default in case someone uses the settings.local.php, but I guess this would be too late.

  2. +++ b/core/lib/Drupal/Core/Fault/Assertion.php
    @@ -0,0 +1,80 @@
    +  /**
    +   * Test to see if a class is being constructed from the allowed scope.
    +   *
    ...
    +   *
    +   * @param string $scope
    +   *   The allowed scope to call the class from. If left null it is assumed
    +   *   the class can only be called from the same namespace or a child.
    +   *
    +   * @return bool
    +   *   Validity of the caller.
    +   */
    +  public static function validCaller($scope = NULL) {
    

    I still think that adding this disallows people from reusing code in ways, we haven't thought of, so do we need that one here?

  3. +++ b/core/lib/Drupal/Core/Fault/AssertionResponse.php
    @@ -0,0 +1,61 @@
    +   *
    +   * This constructor has the job of sorting out PHP's arguments to the assert
    +   * callback (parent::start) since the three callbacks have different argument
    +   * orders.
    ...
    +  /**
    +   * Get a backtrace.
    +   *
    +   * Three ways faults can happen in PHP, three ways you have to get your
    +   * backtrace. PHPsadness...
    +   */
    

    Can this documentation point to the 3 ways how it could happen? Just mentioning them without explaining what they are, is kinda sad.

  4. +++ b/core/lib/Drupal/Core/Fault/BaseFaultResponse.php
    @@ -0,0 +1,314 @@
    +      self::panic("Twig could not start\n\n");
    ...
    +      self::panic('Invalid template: ' . $this->templates . $this->template);
    ...
    +      self::panic($e->getMessage);
    

    We prefer to use static:: over self:: in order to allow subclasses to change the behaviour.

  5. +++ b/core/lib/Drupal/Core/Fault/FaultDiscovery.php
    @@ -0,0 +1,132 @@
    +use Symfony\Component\Yaml\Exception\ParseException;
    +/**
    + * Create a collection of the messages possible for the current fault.
    + */
    +class FaultDiscovery extends \ArrayObject {
    +  protected $root = '';
    

    Ensure to make empty lines after the last use statement, and after the curly brace after the class line

Aki Tendo’s picture

FileSize
45.13 KB

I misunderstood how Sourcetree builds patches. It isn't straightforward. This one will be cleaner.

Aki Tendo’s picture

1.

It would be nice to have it per default in case someone uses the settings.local.php, but I guess this would be too late.

Yes and no. It would be too late for catching problems before the parsing of settings.local.php, but it would catch what comes after. Nothing prevents calling FaultSetup::start(); to reset the FaultSetup, though it currently doesn't expect that - so it wouldn't unregister assertions handlers already registered.

2.

I still think that adding this disallows people from reusing code in ways, we haven't thought of, so do we need that one here?

As I've said before it isn't needed anymore than using the protected and private keywords are needed. But imagine how hard drupal would become to test if ever method and member was public? The Fault system uses this internally to stop the FaultResponders from being invoked without the handler. Since this is a new system that isn't risky. The template patch asserts the AttributeValues are being created by the Attribute class - but I still can't think of a legitimate reason some other class would need to use them. A descendant of Attribute maybe, but the assertion doesn't prevent that.

Others need to weigh in on this though, but my inclination is to just be careful about when and where it is used.

3.

Can this documentation point to the 3 ways how it could happen? Just mentioning them without explaining what they are, is kinda sad.

The callback to the assert handler argument order is ($file, $line, $code, $message).
For errors it's ($code, $message, $file, $line, $context)
For exceptions it's simply ($exception)

I will add @see references for the pages these appear in the php docs.

4.

We prefer to use static:: over self:: in order to allow subclasses to change the behaviour.

Fixed. Initially panic was going to be a private method. It still should be avoided since it's rather unceremonious.

The use statement formatting will also be fixed.

Aki Tendo’s picture

FileSize
45.12 KB
24.8 KB

dawehner 's notes addressed in this patch, plus these patches should be easier to read (I'm getting the hang of this finally)

Now, onto the issue of this assertion


  public static function validCaller($scope = NULL) {

    $trace = debug_backtrace();

    // This function must always be called from assertion.
    if (!$trace[1]['function'] === 'assert') {
      throw new FaultException('mustAssert');
    }

    return static::checkCaller($scope, debug_backtrace());
  }
  /**
   * Divided from above to allow unit testing.
   */
  protected static function checkCaller($scope, $trace) {

    // First level is this function, second the assert. Drop.
    array_shift($trace);
    array_shift($trace);
    // Get this level.
    $origin = array_shift($trace);

    // To find the true origin of the call we have to traverse any
    // override functions by the children. We'll know when we've hit the caller
    // because the function name will change.
    do {
      $frame = array_shift($trace);
    } while ($frame['function'] === $origin['function']);

    // PHP Unit Tests are allowed to avoid this particular assertion.
    if ($frame['object'] instanceof \PHPUnit_Framework_TestCase) {
      return true;
    }

    // Resolve the namespace of the origin.
    $namespace = substr($origin['class'], 0, strrpos($origin['class'], '\\'));

    // Be a little forgiving of devs who start the scope with a \
    if ($scope && strpos($scope, '\\') === 0) {
      $scope = substr($scope, 1);
    }

    // Now do the check.
    return strpos($frame['class'], $scope === NULL ? $namespace : $scope) === 0 ||
        strpos($frame['class'] . '\\' . $frame['function'], $scope === NULL ? $namespace : $scope) === 0;
  }

Again, why run this check? Well without the check we will become unable to reliably change classes that have public methods even if they were never intended to be used outside of their namespace, because we won't be able to know whether or not someone somewhere actually did use them. With this check in place we could make the decision to completely rewrite the Attribute Value classes however we like, or even jettison them from the system entirely, and know they were never used in any module out there (short of someone going out of their way to avoid the assertion, but that's akin to killing kittens).

That's the real reason for adding the check - it keeps the API exposure minimized to ease further improvements by limited possible compatibility breaks. It also makes distinct internal as opposed to external API's - if a maintenance programmer reads that assertion they should know it's not meant to be called from whatever module they're working on.

xjm’s picture

Assigned: Aki Tendo » alexpott
Issue summary: View changes
Issue tags: +Needs committer feedback

Thanks @Aki Tendo! Great work here.

The beta evaluation in the summary is not correct. It says:

Unfrozen because no API changes.

That's not what "Unfrozen" means. In the beta evaluation explanation linked in the summary, "Unfrozen" is listed as issues that only touch: CSS, markup, translatable strings, documentation, automated tests. So I have removed that line from the table.

The disruption section doesn't really seem accurate either. As we discussed in IRC awhile back, this proposed change is somewhat disruptive because it "will require changes across many subsystems and patches in order to make core consistent." At this point in the release cycle, we need to avoid introducing new APIs with only one use, with other APIs that will become deprecated by it also still in use.

The main goal seems to be improving developer experience and reducing fragility through better error handling. (It's not clear to me how this directly improves either usability or performance.) We'd need a committer's signoff for that criteria. So, I'm proposing we get some review from one of the committers before proceeding here in core for 8.0.x. Tentatively assigning to alexpott for that review.

I should say that I am really intrigued by this in any case -- and of course easier debugging makes all of our lives better.

Aki Tendo’s picture

Ok, I'm going to try to make a case for the patch's inclusion - please bear with me cause I'm still trying to get up to speed on things. A bit about myself first - I've been programming PHP for about 10 years now and in that time I've built CMS systems and frameworks from scratch but I've reached a point where I'm no longer really interested in going lone wolf. After a careful survey of the frameworks and CMS's out there Drupal - more specifically Drupal 8 (I don't really care that much for 7 to be honest) lines up with what I want to do. But while learning it I got snagged on a 3 day bug hunt a single assertion could have stopped as outlined in the issue summary.

Drupal has been criticized, repeatedly, for having a ridiculously steep learning curve. This patch, if anything, is trying to lower that curve by setting up a means to manage assert statements. I've went on at length on what assert is and why it's different and in certain situations better than using conditionals with exceptions so I won't repeat myself. I will however bring up a point that got raised over at Sitepoint in an unrelated thread regarding assert.

Drupal quite obviously uses TDD. Assert is usually associated with DBC (Design By Contract) methodology. In the discussion over at Sitepoint the point was made that TDD is a better way all told, to which I countered that there is no reason not to use both in a complementary fashion. TDD is great for unit testing. It's not so good for system testing despite all efforts to the contrary as seen in the recent install bug. Now, would an assert have stopped that embarassing debacle of shipping beta 8 with a broken installer? I don't know, but I'm going to guess probably Unit tests are great for testing interactions within units - they aren't so hot for testing interactions between units. Assert is actually very good at this.

Now, when I spoke of disruption, I was considering what this patch in and of itself changes in the code. Truth be known, until assert statements start showing up elsewhere in the code this patch does nothing. My fear however is this is sort of a chicken and egg problem. Asserts without a handler aren't very useful - they're just another warning that can get thrown. A handler without asserts definitely isn't useful though it is harmless.

As far as implementing assert across the system - I see no reason to rush into them. Further, even the target release for beta was a year away getting asserts everywhere they could go in the code would be unlikely. The adding of asserts is going to be a multiple year process that I wouldn't expect to be fully complete until 8.2 or even later.
Without the handler though there's no impetuous to add them.

Also, I do not agree that "changes across many subsystems and patches in order to make core consistent." will be required. Desirable? Yes. It's also desirable that, now that we are in PHP 5.4 that the array() construct be consistently dropped in favor of [], but that isn't going to happen overnight either. No given assert statement in the system is dependent in any way on any of the others, and phasing them in piecemeal is, in my opinion, the only practical way to go about it. Priority needs to be given to the areas of the code most likely to be mucked up by end developers - the theme engine and then anything else a services.yml file can trash by being misconfigured.

As for how performance would be improved - asserts go away when turned off - so anything they were checking is likewise removed. File exists checks or anything else currently being checked by exception will be sped up by replacing with assert when possible. If a condition gets checked a few hundred times because it's in a loop then the move to assert will be noticeable once asserts are turned off.

The last point - "new APIs with only one use, with other APIs that will become deprecated by it also still in use." - is valid. I could have avoided this by writing just the assert handler without anything that will be supporting it, but then that would create an even worse situation - introducing an API that is going to be dropped within one minor release cycle.

Further, neither the Fault System nor the errors.inc family of functions it will eventually replace are API's in the normal sense. To my knowledge, only the devel module monitors the error handlers and then only indirectly. Since these handlers catch the uncaught errors that come out of the system no part of the system integrates tightly with either. The errors.inc functions do make solid use of the logger - something the Fault system does not yet do for two reasons - one, I'm not that familiar with the logger (yet) and two, I want to maintain the Fault System's independence as much as possible so that it can do its job effectively. That job is to monitor the main drupal system. If nothing goes awry it does nothing and should stay as quite as possible.

During the course of setting this up I looked into two error management systems already out there - Whoops and Boo Boo. Both of those failed this critical litmus test - they don't lazy load - that is they both introduce the full weight of their overhead when in use, and this can't fly. The Fault System has only one file that loads when an error of some sort doesn't occur - FaultSetup. That class does only needs to do one thing - register the handlers. It currently also lets the template engine register templates to handle the appearance of the fault messages, but I'm on the lookout for a better way to accomplish this.

At the end of the day though there is only one goal afoot here - stop the cryptic error messages. Using assert statements along with the fault.yml file method of mapping error codes to verbose explanations of what is going wrong is the best way I can think of to accomplish this goal.

Fabianx’s picture

+1 from a theme system component maintainers perspective to this new system

The Attribute work alone is worth a lot in terms of DX and TX (Themer Experience).

Assertions are really a missing step and being able to _use_ them is huge.

I think of them as a good recommendation, like we slowly used PHP 5.4 syntax for arrays and traits.

Not mandatory, but a good practice.

The Performance benefit is that they don't have any runtime overhead, so some checks that are throwing an Exception can be replaced with an assert can save some performance. So we can harden our systems without overhead! Yeah! :)

And actually that is very useful, too and we used that in some client projects: On production never show a WSOD, but on testing / dev always, so its found by QA, but never shown to end users.

TL;DR:

Because assertions are additional and also don't have any runtime overhead on production, I don't think we need to convert all of core to use them or even be consistent.

But being empowered to use them is a HUGE step forward and with a recommendation to also use assertions we - as core developers - have more possibilities in testing our code.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Fault/AssertionResponse.php
@@ -0,0 +1,22 @@
+  protected $template = 'assertion.twig.html';

That is a nit, but needs to be

assertion.html.twig

according to our naming standards.

Wim Leers’s picture

Aki Tendo: thank you!

Amazing work. Thanks for the thorough analysis, explanations, justifications, patch iterations, and simply thank you for this proposal!

Like others, I'm intrigued, fascinated and enthusiastic about this issue. I'd need to understand the consequences better to write a proper review.

Until I find the time to do that, please remember this: your proposal comes at a tricky time in Drupal 8's life. We're trying to get the release out. Simply for that reason, it may be postponed until 8.1.0. Please don't let this frustrate you. Take a step back before you become frustrated. This being your very first Drupal core contribution, I can't wait to see what else you come up with! Your working/communication style also seems to match that of several major contributors, so I'm sure you'll feel right at home!

alexpott’s picture

I could not agree more with the aims of this patch and the wider initiative. And using asserts is an intriguing solution to the problem. The ability to turn them on in development and off in production is very attractive. The decoupling of the printing of the error from Drupal's theme system is extremely sensible - this has caused us heaps of pain already in 8.0.x.

However according to our beta criteria (as indicated by @xjm in #74) we cannot proceed with this until 8.1.x. Introducing a new system at this point is disruptive to the release cycle. If we commit this patch and then we start using asserts in a low level system like CMI and we discover a critical bug - we have something new to fix before release - either by removing the assert or fixing the assert system. There are other costs too. For example, if we want to support translatable assertions we need to change localise.drupal.org. (That said I'm not too keen on making assertion messages translatable since that just adds another layer of complexity and place for stuff to go wrong and report incorrect errors to the user)

This said I've talked @Wim Leers and @Fabianx in IRC and they are suggesting that asserts could provide a nice way to avoid the performance costs of validating cache tags. If this can be proved, tested and implemented in this patch then we might have a way of moving forward in 8.0.x

Wrt to the patch - there are lots of coding standards things to clean up (for example true => TRUE and PHPDoc things. Also…

+++ b/core/lib/Drupal/Core/Fault/BaseFaultResponse.php
@@ -0,0 +1,270 @@
+  /**
+   * Gather module comments regarding this error.
+   */
+  protected function gatherComments() {
+    if ($this->output['message_code']) {
+      try {
+        // We'll throw out on this line if the service container isn't around.
+        $messages = new YamlDiscovery('errors', \Drupal::moduleHandler()->getModuleDirectories());
+
+        foreach ($messages->findAll() as $module => $message) {
+          if (array_key_exists(array_keys($this->descriptions)[0], $message)) {
+            if (array_key_exists($this->output['message_code'], $message)) {
+              if ($message[$this->output['message_code']] !== $this->output['message']) {
+                $this->output['comments'][$module] = $message[$this->output['message_code']];
+              }
+            }
+          }
+        }
+      }
+      // Nothing to do on this cause except proceed normally. Module comments
+      // are a luxury, not a requirement.
+      catch (ContainerNotInitializedException $e) {
+      }
+    }
+    return $this;
+  }

This is the only dependency on \Drupal or anything in core. Let's remove it and then the library can be in Component. We should invert this and somehow have core inject the necessary information to support this "luxury".

In fact if we are going to do this in 8.0.x I think we should remove the whole error messages stored in YAML complexity and just use strings in the assert() calls. This would also remove a lot of complexity and therefore risk.

It is really exciting that @Aki Tendo has come along with such enthusiasm to improve this in Drupal 8 and with a concrete plan of action. Regardless of whether this makes it into 8.0.x or 8.1.x I think it is a great contribution.

Next steps:

  1. We need a concrete use case where this provides a runtime benefit over assertions and cache tag validation is the most likely candidate - this will determine if it will land in 8.0.x or 8.1.x
  2. Reduce complexity by removing YAML storage of messages
  3. Make the Fault system a component
  4. Fix up the code according to our coding standards
  5. Add documentation to d.o and api.drupal.org (via PHPdoc)

[Edit: improved comment around other costs on doing the patch.]

alexpott’s picture

Assigned: alexpott » Unassigned
Wim Leers’s picture

This said I've talked @Wim Leers and @Fabianx in IRC and they are suggesting that asserts could provide a nice way to avoid the performance costs of validating cache tags. If this can be proved, tested and implemented in this patch then we might have a way of moving forward in 8.0.x

See #2454643: Optimize cacheability bubbling (Cache::mergeTags(), ::mergeContexts(), BubbleableMetadata).

catch’s picture

I don't think we should translate assertion messages. We don't translate exception messages, and PHP error messages aren't localized. This means localize.drupal.org would not need to be updated to support the system (assuming that feature is removed from the patch).

mpdonadio’s picture

@AkiTendo, if you work on the points mentioned above, I would be happy to do a pass for Coding Standards when you are done. I already have my dev environment set up to use them, and I also have the analysis tools installed locally for reviewing project applications. Just let me know.

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: -Needs committer feedback

Updating status, Alex has given feedback in #79.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

I've updated the prioritized changes per the discussion above. Let's also get an update to outline the disruptions and next steps @alexpott mentioned. Great discussion, all.

Aki Tendo’s picture

Well darn it, I had a rather large post setup that got lost when I hit the preview button. :\

Briefer version since I have to get on paying work this morning. I will implement all changes above tonight.

I had a eureka moment while writing this post. The reason for the faults.yml files is to handle large error responses and bypass PHP's 1024 character limit. After all, do we want this in code?

The class member given in the above code is missing, most likely because
it has not been correctly set by the site container when the class was
instantiated. If you are working on a module services.yml file that is the
likely cause.

Your services.yml file should look something like this.

      services:
        MY_MODULE_NAME.twig_extension:
          class: Drupal\MY_MODULE_NAME\TwigExtension
          tags:
          - { name: twig.extension }
          calls:
          - [setGenerators, ['@url_generator']]
          - [setLinkGenerator, ['@link_generator']]
    

The critical lines that can set off this assertion are the two calls
lines. These instruct the service container on which setter methods to
call immediately after building the class but before handing it off to the
requesting class, and what to populate those setters with. Without these
lines the class will be sent to the requester in an incomplete state.

Resources

No. Hence the yaml files and the lookup. But then it occurred to me, why not just link the code like this?

http://api.drupal.org/assertion_codes/Drupal.Core.Template.TwigExtension...

Or some such. This is actually far better in many, many ways:
1) The page could have discussion regarding the error.
2) The documentation would be kept updated even if the user hasn't kept their local install current.
3) Checking server logs could reveal what errors are getting thrown most often.
4) No need for the modules to have a complex means of chiming in.

One small downside, explanations for the module error codes - however that could be worked into the website as well by allowing module maintainers to have pages on their project pages like so...

https://www.drupal.org/project/[official_project]/error_codes/String.The...

Or heck, if the fault responder sees the error string is a URI, use that.

Darn it, I wish I had thought of this sooner - like before I wrote the FaultDiscovery. Oh well, worth the learning experience.

Fabianx’s picture

#86 That wont help you in retrospective, but if you use the http://dreditor.org/ extension there will be a "Restore last input" button, so if you hit preview or any POST form, then you can restore the input :).

Thanks for continuing work on this and using links makes a lot of sense.

If you need help with profiling or finding the validateContexts or validateTags methods, let me know :).

star-szr’s picture

I'd just like to mention briefly on this issue since it's being used as an example: Twig extensions don't need the url_generator and link_generator services, contrib (and core) Twig extensions should extend from the vendor \Twig_Extension class.

Thanks @Aki Tendo for working on this!

Aki Tendo’s picture

I'd just like to mention briefly on this issue since it's being used as an example: Twig extensions don't need the url_generator and link_generator services, contrib (and core) Twig extensions should extend from the vendor \Twig_Extension class.

They should. But as far as I know nothing is stopping someone else from repeating my mistake of extending from the Drupal Twig Extension, and until that mistake is blocked out by declaring the methods of the class final an assertion is needed to alert the dev that they're doing things wrong. That makes this a perfect example of what assert is good for, stopping newbs (like me) from shooting themselves in the foot.

Aki Tendo’s picture

I have completed the changes but not the tests of them and the code formatting pass. Attached is a screenshot of an assertion screen ( the assertion was put in just to test ).

Any ideas on how I can make it look closer to the other error screens?

dawehner’s picture

I really like the initiative as well ... decoupling our error display system from the other part of Drupal is huge time saver in case you would have to debug,
which not only happens on core development but also on development of sites

Just take #2451085: _drupal_log_error() passes NULL to ThemeManager::setActiveTheme(), violating its typehint as example ... it made things impossible to grok for a while.

By decoupling things, for example, we can't rely on a working translation system ... but as others said before, we also don't translate other kind of errors.

  1. +++ b/.htaccess
    @@ -39,6 +39,13 @@ AddEncoding gzip svgz
    +
    +  # Assertions.
    +  # By default PHP has these turned on. Working from the assumption this is
    +  # production, turn them off. Change the string to off and uncomment if you
    +  # want them on but your environment defaults to off for any reason.  If you
    +  # enable assertions, you will also enable the ErrorSystem.
    +  #php_value assert.active                   off
    

    Here is a question: how can a core developer AND the testing environment turn that on? While running the tests we obviously want to have assertions enabled, otherwise part of the point of using it, is lost.

  2. +++ b/core/lib/Drupal/Core/Fault/BaseFaultResponse.php
    @@ -0,0 +1,270 @@
    + * This is the foundation of the Fault system.  The term "Fault" refers to any
    + * form of a runtime exception, be it an error from the PHP engine or
    + * trigger_error, uncaught Exception, or a failed assert statement. We are a
    

    This is something which would be perfect in some documentation on drupal.org. It describes that Fault is the generic term for error, assertion and exception.

  3. +++ b/core/lib/Drupal/Core/Fault/BaseFaultResponse.php
    @@ -0,0 +1,270 @@
    +    catch (\Exception $e) {
    +      static::panic("Twig could not start\n\n");
    +    }
    

    Can we pass along the message coming from $e? Otherwise we potentially loose information, which would be helpful.

  4. +++ b/core/lib/Drupal/Core/Fault/BaseFaultResponse.php
    @@ -0,0 +1,270 @@
    +        // We'll throw out on this line if the service container isn't around.
    +        $messages = new YamlDiscovery('errors', \Drupal::moduleHandler()->getModuleDirectories());
    

    One thing we could do is to add a method onto FaultSetup to pass in the module directories, once they are available, which is after the COntainer is loaded.

  5. +++ b/core/lib/Drupal/Core/Fault/FaultSetup.php
    @@ -0,0 +1,119 @@
    +  public final static function handleAssert($file, $line, $code, $message) {
    ...
    +  public final static function handleError($code, $message, $file, $line, $context) {
    ...
    +  public static final function handleException( Exception $e ) {
    ...
    +  protected final static function runHandler($handler) {
    

    I'd better avoid finals ... it makes itharder to reuse stuff ... you never know whether people might have ideas / uses for it. Just imagine what happens if the fault system is part of a component.

Aki Tendo’s picture

Here is a question: how can a core developer AND the testing environment turn that on?

It's already on for the testing environment, even the one online now (you can test this by submitting a patch with a deliberately failing assertion in the index file). PHP's defaults are assert.active 1, assert.warning 1, assert.bail 0, assert.quiet_eval 0, assert.callback null.

Hence the testing system currently sees assertion failures as PHP E_WARNING errors.

A block of code that needs to test assertions must therefore modify those settings, which can be done at runtime. I've improved the documentation on that block of code - this is its current reading.

  # Assertions.
  # By default PHP has these turned on. Production sites should turn these off.
  # While assertions can be turned off at run time, we need to have this setting
  # in place immediately to catch an assertions thrown before the settings file
  # can be loaded.
  php_value assert.active                   0
  # To enable them, change 0 to 1. Recommended for dev sites.
Can we pass along the message coming from $e? Otherwise we potentially loose information, which would be helpful.

Already done. I was having parse errors in the twig template last night and realized having that info is sorta important.

One thing we could do is to add a method onto FaultSetup to pass in the module directories, once they are available, which is after the Container is loaded.

No longer necessary. For all failures I can see the file path where the error is coming from. I can very quickly discover if the path corresponds to a core element - the path will start with "core." From there rewriting the path to it's api.drupal.org equivalent is fairly painless. If a more detailed error message is wanted the uri to that message should be passed as the first part of the error message - that's where the error reference link in the pic above comes from. The assert that raised that looked like this:

assert('false', 'http://www.drupal.org Another testing message');

As a result, the whole module message / module gathering carnival is avoided.

I'd better avoid finals ... it makes it harder to reuse stuff ... you never know whether people might have ideas / uses for it. Just imagine what happens if the fault system is part of a component.

Normally I would agree, but the plan is the change much of this code in 8.1 - final prevents any code from being written on top of this which would then be broken by the changes that are coming in 8.1. That's the purpose of the keyword - to stop backwards compatibility issues from being created to begin with. Until this code stabilizes it should still be protected in this manner.

Also, as far as limiting people's ideas and uses for code components - there's another side to balance you're overlooking. Any part of the system that is public in this way becomes a future failure point when changed. Code protection through the use of final, private, and protected keywords, and through the Java-protected assertion that's part of this fault system prevents this by separating the internal API from the external one. I want people to reuse code too - but I also want to control how they do it so that changes made in the internal workings of the system do not disturb their code. The only way to do that is to have a complete understanding of how they can interact with the code and only real way to do that is to restrict their access to it to form "the black box" as it were.

For a good example, consider PHP's ArrayObject class. The storage is private - forcing child classes to use getArrayCopy and exchangeArray to manipulate it, and even the offsetSet function must pass through its parent to effect a change on the member. It's annoying, but suppose the core dev's decided to change the name of that internal array from storage to data? A whole lot of code gets broken. But since it's private they have the freedom to make that change.

Aki Tendo’s picture

Assigned: Unassigned » Aki Tendo
Issue summary: View changes

Minor updates to the issue summary.

cosmicdreams’s picture

Is it technically possible to / would it break backward compatibility if we add this system to 8.1.x ? That would be a good test for the kind of advancement our new versioning system should allow.

I want to live in a world where we can add awesome advancements like the one that's being proposed here without having to wait 4 years to make it happen.

Fabianx’s picture

#94 That is the plan:

- Minimal system in 8.0.x if we can prove that it saves performance (and my bet is that it will)
- The originally proposed system with more complexity in 8.1.x

Any chance of a new patch, Aki Tendo?

Aki Tendo’s picture

I'm doing my paying work at the moment. I'll have something out later tonight - about 10 or so hours after this message posts.

As for BC breaks with the 8.1 version - the only module I see being in any particular danger of being disturbed is devel - and I want to work with it's author in developing the 8.1 version, if only for advice.

I'm still going to try to figure out a way to hook in Whoops, BooBoo or any other arbitrary error system handler the user may wish to use, but the default system needs to be one that lazy loads - neither Whoops nor BooBoo do that - they load themselves into memory in full during setup.

alexpott’s picture

re #90 I don't think what it looks like should be a concern. We should make a design decision a same that the assertion message is displayed according to the browser defaults for html tags. This way it does not have dependencies on anything and can be used reliably before anything Drupal is really set up.

Also I think the idea of linking to a page on d.o is a good one but the priority should be a good message. Before we could turn on such a link we'll need to make changes to d.o - eg. add a new node type and decide what fields it has etc.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
24.91 KB
67.3 KB

Ok, here we go.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
24.92 KB
1.51 KB

Didn't know the comment docs could cause fails.

dawehner’s picture

FileSize
11.55 KB

... As promised on IRC

Crell’s picture

I think there's a couple of points here that are being mixed in together; they all really deserve to be treated separately, in part because many overlap with existing code and direction (but not all together).

The various moving parts are:

Claim 1: Error messages and error handling should be useful for figuring out what a developer did wrong so they know how to fix it without having to guess.

I don't think it's mathematically possible for me to agree with this statement any stronger!

Claim 2: Drupal should use PHP's assertions features more aggressively (read: at all) to identify issues that are undeniably programmer error.

I started off reading this thread in the "pfft, that's what Exceptions are for" camp, but Aki has made a good argument here for them being different. This discussion is distinct because PHP already has handling for failed assertions; it's just very minimalist. Doing something fancier with it is optional and is, by design, distinct from adding assert() statements to the codebase. Ie, we could start adding assert() calls to the codebase right now and they'd behave exactly as expected. (And it would have zero impact on release blocking-ness, other than surfacing more bugs in our existing code.)

Side note: According to 3v4l.org, HHVM doesn't do assertions: http://3v4l.org/2Ke3T (Or maybe they're just off by default in whatever their configuration is?)

The main counter-argument I'd make is that there are cases where a fatal error in Drupal can leave the system in an unusable state. I ran into that with this bug in Entity API (#2457427: Bad error handling of invalid Entity definition), where a fatal occurs on cache rebuild, which happens on module install, and you can't uninstall a module without the cache being rebuilt, so the site is completely dead until the code bug is fixed. (Which I couldn't find because error reporting is off by default in Drupal 8 now. Score!) So let's not be too quick to say such issues should be fatal when that may even make recovery harder, not easier.

Claim 3: There should be some additional utility functions for use in assert() statements.

(The Drupal\Component\Fault\Assertion class.)

Debatable case by case, but this is not even a Drupal-specific question. A collection of "useful assert utilities" can, and I would argue should, live outside of Drupal entirely as its own Composer library that we can then pull in. Perhaps it's even something the PHP League would be interested in hosting.

Claim 4: Assert fails should be prettily formatted using Twig.

I have no strong feelings on this one, other than, as others have noted, the need to be very fundamentalist about decoupling in such a system. Twig implies file system reliance, and autoloading, and, well, Twig. Maybe not the biggest dependencies but for assertions I'd expect something more "raw". Also bear in mind that these messages would be intended for developers actively writing code, so flexibility or customizability are not priorities. The only "formatting" I'd think we need to consider is based on the response type; eg, an Ajax request or a JSON request would need to wrap the message in a JSON object to avoid the (many) problems that plagued Drupal 7 in that regard. (And even Drupal 8 to an extent, too. Try adding a block from the UI when there's an error somewhere. It's extremely unhelpful.)

Looking at the template, actually, it's basic enough that I don't know why we'd even bother with Twig. An inline HEREDOC block and a concat or two would probably be sufficient.

Claim 5: Assert fails should be routed through Symfony's Response objects.

I'm not convinced here either, for many of the same reasons as Twig. This is code that would run outside the context of a Kernel. Thus there's no hard-need (as there is in 99.9% of the rest of the codebase) to ensure we end up returning a Response object that index.php can send(). It's a completely different pathway, so I don't see much advantage to that coupling. Most of what the Response object does for us (encapsulation, header standardization, cache normalization, etc.) is not relevant in this context. The only thing that is relevant would be content negotiation to determine whether to return a string or JSON object or XML fragment that the client would understand... but the Response object doesn't do that for us anyway. So I don't think this is a good coupling in this case (whereas it would be a requirement in almost any other case).

Claim 6: We should (in 8.1) have a unified error, assertion, and exception response object.

Strongly disagree, for the reasons Aki already explains: Exceptions and assertions are different things!

We already have a very robust way to handle exceptions that happen within the kernel, which is 99% of them. We don't need to bind that to assertions. That would be a regression.

Our handling of trigger_error() errors right now is rather crappy, and I don't think anyone would debate that point. I question if that needs to be integrated with assertions, though.

In Summary

All that said, I am warming to the idea of using assertions, which I did not expect, and I am jumping up and down and cheering at the idea of error messages that tell you how to fix your code (whatever error mechanism they use). However, I believe even the latest, much smaller patch is excessive for this case and it could be made much, much simplier and with fewer dependencies and places for things to go wrong.

I would ask that we split the "assertions: yay or nay?" question to a separate thread from "let's build an assertion handler", since we could answer the first very easily without having to consider the second since PHP already has a default one that can do the job.

And now, a code review because why not? :-)

  1. +++ b/core/lib/Drupal/Component/Fault/Assertion.php
    @@ -0,0 +1,90 @@
    +  /**
    +   * Test to see if a class is being constructed from the allowed scope.
    +   *
    +   * In Java a protected method is callable by other classes in the same
    +   * package, or namespace. This assertion is meant to create the same
    +   * restriction, or an even tighter one if desired.
    +   *
    +   * This is done for the same reason the protected keyword is used. Protected
    +   * methods can be changed without fear of creating backwards compatibility
    +   * headaches. This further separates the internal API from the external one.
    +   *
    +   * @param string $scope
    +   *   The allowed scope to call the class from. If left null it is assumed
    +   *   the class can only be called from the same namespace or a child.
    +   *
    +   * @return bool
    +   *   Validity of the caller.
    +   */
    

    The very concept here introduces a huge set of questions, as this is very different to how PHP normally works. It almost becomes a language extension by assert(), in a sense. Do we even *want* people to be able to limit classes to just their own namespace? I'm of at least 3 minds on that question at the moment all by myself. :-)

  2. +++ b/core/lib/Drupal/Component/Fault/BaseFaultResponse.php
    @@ -0,0 +1,228 @@
    + * The term "fault" refers to any form of runtime problem, be it an error from
    + * the system or trigger_error(), an uncaught exception, or an assert failure.
    + * At present this system only deals with assert failures - it will be expanded
    + * in 8.1 to deal with errors and exceptions and replace the existing code
    + * managing those areas.
    + */
    

    Symfony already has a fairly robust handling of exceptions and their resulting responses, and we already have a system in place for handling those via listeners. A Grand Unified Theory of Faults smacks of more coupling than we want to have. Ie, I would NOT want this to also encompass exceptions, as they are (as you say) different beasts for different purposes and should not be coupled together.

  3. +++ b/core/lib/Drupal/Component/Fault/BaseFaultResponse.php
    @@ -0,0 +1,228 @@
    +  /**
    +   * Figure out which API pages are most relevant to the current error.
    +   */
    +  protected function referenceLookup() {
    

    Now *that* is a classy idea, all on its own! I like!

  4. +++ b/core/lib/Drupal/Component/Fault/BaseFaultResponse.php
    @@ -0,0 +1,228 @@
    +  public function send() {
    +    if ($this->isXmlHttpRequest || PHP_SAPI === 'cli') {
    +      print ("Assertion Failure: Asserted {$this->output['code']} - {$this->output['message']} (line {$this->output['line']} of {$this->output['file']}).");
    +    }
    +    else {
    +      parent::send();
    +    }
    +  }
    

    This is a bad idea. If the request is xmlHttpRequest, then odds are you want to send back a JSON-formatted error. Send back a plain text error and currently Drupal's Javascript swallows it whole and reports nothing. (At least it did when I last ran into such issues; if it's improved since then, yay.) It makes sense for CLI, but not Ajax.

  5. +++ b/core/lib/Drupal/Component/Fault/FaultSetup.php
    @@ -0,0 +1,46 @@
    +  public final static function handleAssert($file, $line, $code, $message = NULL) {
    +    static::runHandler(new AssertionResponse($file, $line, $code, $message, debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)));
    +  }
    

    These can easily be normal methods rather than static. The ASSERT_CALLBACK should be able to handle being called with [$this, 'handleAssert']. The start function being static is, I think, acceptable.

  6. +++ b/core/lib/Drupal/Component/Fault/FaultSetup.php
    @@ -0,0 +1,46 @@
    +  private final static function runHandler($handler) {
    

    Calling this parameter $handler is very confusing, as it's not a handler but a Response instance. Call it $response. I spent 5 minutes trying to figure out what handler service you were routing to. :-)

  7. +++ b/core/tests/Drupal/Tests/AssertionTestingTrait.php
    @@ -0,0 +1,106 @@
    + * @code
    + * public function setUp() {
    + *   $this->startAssertionHandling();
    + *   parent::setUp();
    + * }
    + * @endcode
    + */
    +trait AssertionTestingTrait {
    

    I can definitely see this trait being useful if we start using assertions in the code.

    However, the code sample likely shouldn't call parent::setup() but just a // ... for whatever other setup someone has to do. Many test cases won't even have a parent class.

  8. +++ b/core/tests/Drupal/Tests/AssertionTestingTrait.php
    @@ -0,0 +1,106 @@
    +  /**
    +   * Collections of captured assertions.
    +   */
    +  protected $assertionsThrown;
    

    Is "thrown" really the right verb for assertions? I'd think "flagged" or "raised". "Thrown" I've only seen used in the context of exceptions.

  9. +++ b/core/tests/Drupal/Tests/AssertionTestingTrait.php
    @@ -0,0 +1,106 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function tearDown() {
    

    Since many test classes may have a tearDown() method already, it may be useful to give this a different name that people can call from their tearDown() method. Otherwise classes that have a tearDown() and use this trait will have to do more esoteric contortions to make it work. (I think you have to alias the method when importing the trait, then call that. Icky.)

Aki Tendo’s picture

Tackling the above post points one at a time

Side note: According to 3v4l.org, HHVM doesn't do assertions: http://3v4l.org/2Ke3T (Or maybe they're just off by default in whatever their configuration is?)

Per the manuals assert is supported

http://docs.hhvm.com/manual/en/function.assert.php

But it has a buggy history...

https://github.com/facebook/hhvm/issues/2279

where a fatal occurs on cache rebuild, which happens on module install, and you can't uninstall a module without the cache being rebuilt, so the site is completely dead until the code bug is fixed. (Which I couldn't find because error reporting is off by default in Drupal 8 now. Score!) So let's not be too quick to say such issues should be fatal when that may even make recovery harder, not easier.

Assert shouldn't be used to check code integrity during module load and setup since that can go wrong by getting a bad module. Assert is for conditions which indicate a problem in the code itself, not to test for problems in outside code (including 3rd party modules), connections et al, that's what exceptions are for. And as much a fan of assert as I am, when in doubt use an exception throw or error trigger.

Debatable case by case, but this is not even a Drupal-specific question. A collection of "useful assert utilities" can, and I would argue should, live outside of Drupal entirely as its own Composer library that we can then pull in. Perhaps it's even something the PHP League would be interested in hosting.

Perhaps. It largely depends on how many we have. There's also merit to the criticism that this sort of thing creates a hard (worse unnecessary) dependency on the class that holds the assertion library.

I have no strong feelings on this one, other than, as others have noted, the need to be very fundamentalist about decoupling in such a system. Twig implies file system reliance, and autoloading, and, well, Twig. Maybe not the biggest dependencies but for assertions I'd expect something more "raw". Also bear in mind that these messages would be intended for developers actively writing code, so flexibility or customizability are not priorities. The only "formatting" I'd think we need to consider is based on the response type; eg, an Ajax request or a JSON request would need to wrap the message in a JSON object to avoid the (many) problems that plagued Drupal 7 in that regard. (And even Drupal 8 to an extent, too. Try adding a block from the UI when there's an error somewhere. It's extremely unhelpful.)

Looking at the template, actually, it's basic enough that I don't know why we'd even bother with Twig. An inline HEREDOC block and a concat or two would probably be sufficient.

That can be stripped if desired. It was placed in there largely as an attempt by myself to do things "the drupal way" as much as possible. That said, the Fault system loads after the autoloader and is dependent on symfony, but not Drupal. Do we want to have it utterly independent of Symfony as well? I can do that, but it will need to register it's own autoloader and be directly required from index.php in order to have that independence.

Routing through symfony's http kernel likewise occurs because I was following the existing pattern for consistency. I can remove this easily if that's desired.

Strongly disagree, for the reasons Aki already explains: Exceptions and assertions are different things!

We already have a very robust way to handle exceptions that happen within the kernel, which is 99% of them. We don't need to bind that to assertions. That would be a regression.

Our handling of trigger_error() errors right now is rather crappy, and I don't think anyone would debate that point. I question if that needs to be integrated with assertions, though.

Fault handling has a huge amount of overlap. When active, asserts affect the system as errors do. Unhandled exceptions may as well be fatal errors as far as the run time is concerned. While these three methods are used in differing ways, gathering and logging their results is much the same. Also, I'm not convinced the Exception handling system is that well thought out or robust because the index.php file runs the drupal code in a try/catch block. There should be no need of this - if the final exception handler was robust and dependable then anything reaching all the way back out to index might as well be allowed to hit the final handler.

Trigger Error needs to be heavily rethought - there's a world of difference on how Deprecation notices are handled from warnings and fatals. In my mind...

* Deprecation - Warning of things to come - devs should take seriously but end users should be safe ignoring them and run error_reporting(E_ALL & ~E_DEPRECATED & ~ E_USER_DEPRECATED)
* Notice - Anomolies which indicate a problem that may be inconsequential or might balloon out into something far more serious, like an unset var.
* Warning - Why PHP doesn't go ahead and crash on these I don't know. I've never seen a script satisfactorily conclude if a warning gets thrown.
* Fatal - No hope of continuing, at all. trigger_error('', E_USER_ERROR) is fatal and should be used instead of an exception throw when you need to make certain that drupal stops with no hope of recovery because of the situation at hand. Never rely on someone down the chain knowing better than to write the over-greedy catch ( \Exception $e) block.

In a sense, Exceptions throw/try/catch is a sort of super control structure (and, when abused, can end up reading like control structures). Trigger error is a form of logging and emergency shutoff switch. Assertions provide a layer of analysis that can be turned off when not needed or wanted. But once thrown and when they hit one of the three final handlers the resolution is the same -- log the event, bring the system to a stable stop and present the user or dev information concerning the event commensurate to the amount of verbosity desired for the system. And frankly, that process does need to be consistent and air tight, however it is done.

The very concept here introduces a huge set of questions, as this is very different to how PHP normally works. It almost becomes a language extension by assert(), in a sense. Do we even *want* people to be able to limit classes to just their own namespace? I'm of at least 3 minds on that question at the moment all by myself. :-)

To the issue of the soundness of the concept - Java has always treated the protected keyword this way. Always. So I feel the computer science theory is sound here. It would be very nice if the language itself would provide this level of access formally, but at this point PHP 7 is feature frozen and this would require a new keyword so we're looking at another 5 to 10 years before it could even be considered.

Like many things in CS using this construct involves a tradeoff. If you setup a class with this assertion you lose the ability to use it in novel and clever ways. In exchange you reduce the exposed API of the system further while introducing a measure of flexibility to the design of the internal API's structure. This assertion, if allowed into Drupal, will be used as protected is used in Java - to prohibit otherwise public class methods (including, especially, constructors) from being called from elsewhere in the system. That means the redesign and rebuilding of those methods within the namespace can be carried out in a far more predictable fashion with regards to backwards compatibility.

This is a bad idea. If the request is xmlHttpRequest, then odds are you want to send back a JSON-formatted error. Send back a plain text error and currently Drupal's Javascript swallows it whole and reports nothing. (At least it did when I last ran into such issues; if it's improved since then, yay.) It makes sense for CLI, but not Ajax.

I'm not familiar with the JS side of the equation, I just mimicked verbatim what _drupal_log_error currently does.

Calling this parameter $handler is very confusing, as it's not a handler but a Response instance. Call it $response. I spent 5 minutes trying to figure out what handler service you were routing to. :-)

The name comes from the PHP functions that are doing the firing -- set_exception_handler, set_error_handler and assert_options(ASSERT_CALLBACK). They may be response instances, but they are the callbacks for those internal handlers. They are static methods within FaultSetup so that no other code has to be evaluated until an error actually occurs. Once that happens their only job is to sort out the inconsistent argument order that PHP has for the callbacks. Run handler creates the object then invokes its methods in sequence so that, individually, those methods can be checked in unit testing.

However, the code sample likely shouldn't call parent::setup() but just a // ... for whatever other setup someone has to do. Many test cases won't even have a parent class.

All the ones I've written so far descend from Drupal\Test\UnitTestCase and it does have a setup method. Interestingly, it doesn't have a tearDown and the number of tests I've written that require specific teardowns are, relatively, rare. Aliasing the method isn't that hard to do and would likewise be sufficiently rare that it wouldn't bother me. But I'll bow the the wishes of the community at large on this.

Crell’s picture

Assert shouldn't be used to check code integrity during module load and setup since that can go wrong by getting a bad module. Assert is for conditions which indicate a problem in the code itself, not to test for problems in outside code (including 3rd party modules), connections et al, that's what exceptions are for. And as much a fan of assert as I am, when in doubt use an exception throw or error trigger.

Agreed. We will need to put very clear documentation and guidance around this point, because Drupal developers simply love to find a new shiny and apply it to ALL THE THINGS without understanding how appropriate it is or not. :-) I fear the number of people who will, in all earnestness, start using assertions as "easier to type exceptions" if we don't provide very good guidance.

Regarding exceptions, there's an important distinction regarding how exceptions and Symfony interact right now. As short a version as I can manage:

- index.php creates a DrupalKernel.
- DrupalKernel does various bootup tasks, then passes off to HttpKernel.
- HttpKernel runs entirely inside a try-catch block. 99% of Drupal happens within this try-catch, and any exception thrown there will get handled by it.

index.php has some catch blocks in it exclusively to handle part 2: The pre-HttpKernel bootup. That's only very low-level errors. The most common is "your container is out of sync, yo!", which is what the second catch is all about. The first one, catching HttpException, uh, I don't know where it came from. This is the first time I've seen it. :-) I suspect it's there for the cache system, I suppose? I don't know what else would throw a pre-HttpKernel HttpException.

In any case, the vast majority of exceptions are already handled by the HttpKernel Exception event, which results in a Response object, which send()s normally. They don't need any additional wrapping or templating or formatting. The scant few that are not are very low level (as above) and... don't need any templating.

So while in concept I can see the similarities between various "fault" cases, in practice there is no real reason to merge them and plenty of reasons not to. (Mostly, coupling and complexity.) Most fault cases can/should be exceptions, not fatals, because they can then still flow through the Symfony response pipeline and can be returned from the kernel handle() method and send()ed properly, which therefore allows $kernel->terminate() to work (where we do some rather important cleanup). In the case of assertions, though, the whole point is to fatal out and say "screw you, developer, fix your code right now because this makes *no sense at all*". That doesn't need to go through a nicely formatted response/send/terminate process. NOT doing so is the entire point. :-) (If you did want that... throw an exception.)

So given that, I see both the Twig and Symfony dependencies as unnecessary. In fact, I'd put the question back on you and ask "so what is wrong with the PHP default handling of assertions that we need to do anything at all?" An ugly "here's what's wrong, fix it damnit!" seems entirely appropriate.

I don't know why you'd need your own autoloader, though. The autoloader is Composer, not Symfony, and you shouldn't have to think about that at all. (Unless you want to handle assertions within Composer itself, which I doubt.)

One other note:

To the issue of the soundness of the concept - Java has always treated the protected keyword this way. Always. So I feel the computer science theory is sound here.

I must chuckle at "Java does this" and "the computer science theory is sound" in the same sentence. :-) There's a LOT that CS folks cringe at in Java. (Not to say they don't in PHP as well, but Java == CS-blessed concepts is simply not the case. I don't know that any language can make that claim with the possible exception of Haskell.) That's a rather poor choice for Appeal to Authority.

That said, with PHP 7 having anonymous classes and there being a good chance that 7.1 will have inner classes, that use case seems like it will be covered already. (Even if I am not entirely in favor of either feature.)

mpdonadio’s picture

The first one, catching HttpException, uh, I don't know where it came from. This is the first time I've seen it. :-) I suspect it's there for the cache system, I suppose? I don't know what else would throw a pre-HttpKernel HttpException.

It was introduced in #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003 to allow the kernel to abort on a bad HTTP request, in this case a malformed Host header that could cause a DoS. The exception is there to allow an unbooted kernel to generate a response with the proper HTTP code instead of the 500 from the second catch. This mechanism is also used in #2221699: HTTP_HOST header cannot be trusted to deal with spoofed headers. #2389811: Move all the logic out of index.php (again) is an attempt to clean up this mess.

Crell’s picture

Ah, thanks mpdonadio. I believe my previous statement regarding it being crazy-edge-case-only cases is still valid, then.

Aki Tendo’s picture

So while in concept I can see the similarities between various "fault" cases, in practice there is no real reason to merge them and plenty of reasons not to. (Mostly, coupling and complexity.) Most fault cases can/should be exceptions, not fatals, because they can then still flow through the Symfony response pipeline and can be returned from the kernel handle() method and send()ed properly, which therefore allows $kernel->terminate() to work (where we do some rather important cleanup). In the case of assertions, though, the whole point is to fatal out and say "screw you, developer, fix your code right now because this makes *no sense at all*". That doesn't need to go through a nicely formatted response/send/terminate process. NOT doing so is the entire point. :-) (If you did want that... throw an exception.)

You're conflating fault creation with logging. They are two entirely separate issues and very different issues. Until you can make that division clear in your head your comments to the complexity of their merger are moot, primarily because there is no merger intended at all.

The Fault System does not in any way need to interfere in any way with whatever existing try/catch hierarchy is in place for Drupal or Symfony. It only comes into play when an exception escapes that hierarchy uncaught and reaches the top stack frame. The PHP engine will, at that point, call the callback method set by set_exception_handler. Keep in mind that the fact the exception bubbled up to this point is already a fatal condition - the code has no means to return to normal code flow and will terminate at the conclusion of the callback function.

Errors and asserts are trickier, because they immediately go to the handler - their is no interceding try/catch network. That handler has one of two choices, it can halt program flow or it can allow it to continue beginning at the statement which follows the error or assert. Assert differs from errors only in its ability to be turned off.

In a sense, the fault system is to DrupalKernel what a BIOS is to a real kernel. Under normal operation it should come up, get ready to catch problems, but not actually be called on to do so. When errors, assertions and exceptions are caught the log and report process is *exactly* the same. There's only one wrinkle caused by PHP giving the handlers different argument orders. This is what that makes the 8.1 code look like...

 /**
   * Assertion Handler
   *
   * @see http://www.php.net/assert
   * @see http://www.php.net/assert_options
   */
  public final static function handleAssert($file, $line, $code, $message) {
    static::runHandler(new AssertionResponse($file, $line, $code, $message, debug_backtrace()));
  }

  /**
   * Error Handler.
   *
   * Note - this code is not yet in use. It will go into effect in 8.1
   *
   * @see http://www.php.net/set_error_handler
   */
  public final static function handleError($code, $message, $file, $line, $context) {
    static::runHandler(new ErrorResponse($file, $line, $code, $message, debug_backtrace(), $context));
  }

  /**
   * Exception Handler
   *
   * Note - this code not yet in use. It will go into effect in 8.1
   *
   * @see http://www.php.net/set_exception_handler
   */
  public static final function handleException( Exception $e ) {
    static::runHandler(new ExceptionResponse(
      $e->getFile(),
      $e->getLine(),
      $e->getCode(),
      $e->getMessage(),
      $e->getTrace(),
      $e )
    );
  }

  /**
   * Run a Fault Handle and return its response, then exit system error code 1.
   */
  protected final static function runHandler($handler) {
    $handler->clearBuffer()
      ->compileResponse()
      ->prepare(Request::createFromGlobals())
      ->send();
    exit(1);
  }

With all three handlers now seen it should be clearer that the only way the operation differs is that the callback must reorganize the arguments to a consistent order for the responder. The error handler has a tad of logic for dealing with the error context, and the exception handler for handling data provided by the exception object, but both of these are fine as children of a base fault handling process.

Aki Tendo’s picture

Issue summary: View changes
Crell’s picture

OK, so you're only talking about the "crazy edge case" exceptions that already are outside the scope of HttpKernel as described in #104. That was not clear previously. In that case, I can see the argument for unifying their handling perhaps. However, I think it only further strengthens the position that we're talking about a case that should have absolutely on dependencies at all on Symfony, Twig, or anything else. All it should care about is if it should return a string, JSON object with string, or XML object with string.

That said... I don't believe the question has been answered yet: What is wrong with the default PHP assert() output that needs fixing here? Also, you need to also keep in mind that XDebug handles a *lot* of stuff for us already. I would favor "use XDebug" over "write new code" if at all possible. So again, what is the hole in what PHP/XDebug does natively that needs to be addressed?

Aki Tendo’s picture

  1. Xdebug is not universally installed - so its features are moot - drupal installations should not expect to be running or even be developed under it. To say otherwise is a bunch of elitist tripe. Most people still come into PHP and drupal without a full understanding of what it can do and they start tinkering with it long before they learn of the existence of, let alone how to use, xdebug.
  2. If an assertion fails on code running before drupal sets up its handlers (which currently is VERY late in the setup process for an error handling system) and the assertion that is protecting against fatals out then you'll either get PHP's usual cryptic garble or, if you turned php display errors off then welcome to the drupal white screen of doom. The fact that drupal can (and often does) die with no error message or log should be an embarrassment.
  3. No existing code out there is going to perform the primary task of this system - figure out the correct API screen for the class and method the error occurred in, and give the writer of the assertion or error message the option to place a uri in the error message and expect it to be linked up on the error screen in html mode. That may not seem like much to a multi-year veteran of Drupal, but to someone who knows next to nothing about drupal and is just starting out it is invaluable.
  4. The 8.1 version will have the ability to safely disregard E_USER_DEPRECATION notices, or display them in the messages area at the admin's option. Once all of these deprecated calls are cleared out of core that will leave only the ones in 3rd party modules, which will give them the ability to know that their module is in danger failing to work in the near future without failure. With a little work this could also be extended to be able to set up an upgrade watch system to warn the admin when a module must be upgraded before an upgrade can safely be proceeded with.
  5. I've not seen anything Whoops or BooBoo can do that Symfony var-dumper cannot. My original intention for this add on was to use that code to display the fault traces, but in the interest of getting something able to be put out with 8.0 that library was pulled even though it is part of Symfony 2.6 core.
  6. Much of what I want to do is aimed at trying to simplify the mystery of errors and their presentation - dumb them down. I'm amazed at the amount of push back on this relatively simple goal - of making life easier on newcomers. "You shouldn't do that, xdebug can cover that case," and so on - again have you ever stopped to consider the simple truth that someone starting off may have no idea of how to use xdebug? If Drupal becomes any more complex it might as well be rewritten in C++ cause no one without that skill level will be able to deal with it anymore.

There, that's as plain as I can put it. At this point I'm actually getting upset, and I didn't start this project to be badgered this way. And, to be blunt, what this does isn't for the guy that's worked with Drupal since version 2, has mastered git, drush, xdebug and all the fixins and can call to mind what might be going wrong when code fails in any part of the system. This project is meant to try to make things easier on the beginners and novices.

almaudoh’s picture

what this does isn't for the guy that's worked with Drupal since version 2, has mastered git, drush, xdebug and all the fixins and can call to mind what might be going wrong when code fails in any part of the system. This project is meant to try to make things easier on the beginners and novices.

+1. IMHO this should be sufficient reason to evaluate this project for its own merits. Maybe even getting some parts of it into 8.0.0

Fabianx’s picture

In the interest of getting this back on track and I believe this is a good direction and D8 should be more DX friendly, lets take a look at Alex' list again:

  1. We need a concrete use case where this provides a runtime benefit over assertions and cache tag validation is the most likely candidate - this will determine if it will land in 8.0.x or 8.1.x
  2. Reduce complexity by removing YAML storage of messages
  3. Make the Fault system a component
  4. Fix up the code according to our coding standards
  5. Add documentation to d.o and api.drupal.org (via PHPdoc)

I think all of that is done - except the performance benefit and maybe some code nit picks.

@Aki Tendo:

Could you make a patch next that replaces the call to:

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Cache!CacheContex...

in mergeContexts

AND

the call to

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C...

but only in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C... for now

with assertions.

We can then benchmark the patch and show that we get a performance benefit as well as a DX benefit by using assertions and not throwing an Exception.

Aki Tendo’s picture

I will incorporate those in the next set of patches. The next patch (code is done, starting with the test revising) removes all outside dependencies, although it does look to see if var-dumper (or anything else) has defined dump - if it has it calls it.

It also performs all its logging now without the help of the logger class.

Aki Tendo’s picture

I had a bad git crash that wiped out my commit history, so I can't build an interdiff for this -- not that it would help because once again massive changes done - Twig and Symfony have been removed. I doubt trying to follow an interdiff between this and #100 would be helpful

Also attached is the cache optimize patch that Fabianx requested. It runs independently. I don't know what tests it will break (if any).

Keep in mind to do performance testing the cache patch must be tested with assertions turned on (the default) then off in turn. The efficiency pickup will be realized when they are turned off.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
46.16 KB
2.43 KB

Guess I need to silence logging during auto-testing.

Fabianx’s picture

Status: Needs work » Needs review

Can we create a patch with the assert optimize on top of the fault one?

We also need to check how to ensure tests generally continue to work, so that we can test failure conditions using assert.

Aki Tendo’s picture

Switched out sad droop to svg, removed the tests being disrupted by having slightly different php versions, and providing a combined patch.

Aki Tendo’s picture

Trying the cache patch again.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
46.07 KB

Ok, I solved the problem. Unfortunately, until the cacheContexts::validateTokens is rewritten assert can't be used to avoid it. The function is misnamed - it not only validates, it caches tokens.

Attached is a working patch that applies what assertions are possible with it, but the speed up is negligible that I could see. Then again, I'm not that good at profiling.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
48.94 KB

The one failing test had to be rewritten with the AssertionTrait in use. Also had to adjust the logic of that Trait to, when necessary, throw an exception to halt the test on assertion fail to prevent proceeding onward to a block of code that, because of the assertion failure, contains a Fatal Error.

If the test runs successfully I'll clean the code up tomorrow.

Fabianx’s picture

Aki, it is fine to remove the caching of the validated contexts once they are converted to an Assertion. The caching code was only needed, because it was run at run-time.

Aki Tendo’s picture

FileSize
51.14 KB

The patch I thought would work last night failed because for whatever reason the AssertionException wasn't included in the patch.

Aki Tendo’s picture

Status: Needs work » Needs review
Aki Tendo’s picture

Faults patch updated with the changes to the assertion trait,
Cache tests further optimized, compare with and without assertions on.

Aki Tendo’s picture

Renderer test base has a mock cacheContexts object. Method validateTokens has to be configured to return TRUE for the tests to run correctly, otherwise the assertions will always fail.

Fabianx’s picture

#133 If you feel those patches are ready, I can take on the profiling. Are they ready?

Aki Tendo’s picture

Yes.

Aki Tendo’s picture

Fabianx’s picture

Unfortunately devel_generate 8.x is broken right now and on node/add/article I had WSOD, so I am testing with just a front-page, which is probably a bad example, but even then we save on average:

1 ms of 45 ms (with xhprof disabled) == 2%, with xhprof I also saw it be 2% better.

The counts are -164 calls to validateTags() and -131 calls to validateTokens, so very unrealistic compared to what dawehner saw as call counts.

This is just for an initial benchmark. I will ask plach on Monday or Tuesday how he generated his one test scenario, whose dump I could not use anymore due to missing upgrade path (which is totally fine).

I can already tell that seeing assert == 0 time is nice though :D.

willzyx’s picture

Fabianx’s picture

#138: Thank you very much! I will try that!

aspilicious’s picture

Can we have some screenshots in here?
And a way to trigger all the visual goodies for non backend experts?

In the end this will ned a signoff from the frontend people so they need a way to test this.
They also could help than with some small adjustments if needed.

I'm kinda afraid of new visual assets like

  • sad-droop.svg
  • drupal-error.png

Why is one an svg and the other a png. We already have a bunch of icons in core, why would we introduce new ones?
On top of that, this should follow our css coding standards. https://www.drupal.org/node/1887862
The one thing I noticed immediately were the "color notations".

I think the plan should be the following for THAT part of the patch
1) Gather a bunch of screenshots and post them in here
2) Collect some info from our UX and frontend team
3) Implement the changes

Last remark, are those inline templates accessibility proof or doesn't that matter for this.

aspilicious’s picture

Reading as " drupal developer" who is not familiar with all the technical details the code/comments sometime reads a bit strange.
So I think we need our doc team to review some of the texts. Just like we did with some of the caching patches Wim made.

For example

+    // Idiot Proof.
+    if (count($trace) === 0) {
+      throw new FaultException('Why are you asserting this in the global namespace???');
+    }

I suspect that eventually someone will feel offended by this. And we usually don't yell at people in comments. "???" feels very aggressive for some.

One code remark. I would rename the $html property in the FaultSetup class.
Because it's not really html that you should put in there, it's a file containing html.

I know it can be very frustrating to start with Drupal development certainly when it's such a big one to start with.
That's why I'm adding these comments to help you forward.

Aki Tendo’s picture

Screenshot attached (There's really only the one possible, though the information conveyed will differ depending on the failure). There is only one new visual asset - the sad-droop.svg which replaced the .png file during development.

The easiest way to test this is to add the line "assert(false, 'test');" after the FaultSetup::start(); line in the index.php file.

My understand is the CSS coding standards are for code that is going to be extended by theme makers. Nothing here should be extended because if the system is working correctly this patch won't output anything (or do anything).

As far as accessibility concerns on the inline templates - if you are seeing what this patch can output for any reason you've got a much bigger problem with your system than accessibility.

Aki Tendo’s picture

I suspect that eventually someone will feel offended by this. And we usually don't yell at people in comments. "???" feels very aggressive for some.

That particular assertion is there more for the unit test than the outside code - I made a mistake with my sample backtrace that, without that check, sent the system into an infinite loop. The idiot I was proofing against is me.

Actually tripping that check by someone else would require calling it from the index.php file or another front controller if one exists. So you really, really have to try to get that to show up. Still, I'll go with whatever verbage is desired - such is not my area of expertise.

aspilicious’s picture

Well now you're really underestimating the process for frontend changes/additions.

here is only one new visual asset

Well than we have a bug ==> '/core/lib/Drupal/Component/Fault/drupal_error.png'

My understand is the CSS coding standards are for code that is going to be extended by theme makers. Nothing here should be extended because if the system is working correctly this patch won't output anything (or do anything).

Thats not how it works. *Everything* should follow the standards, else it is not a standard.

if you are seeing what this patch can output for any reason you've got a much bigger problem with your system than accessibility.

Some developers have accessibility issues they need to understand the message as discussed in IRC. We need a signoff from them.

And I'm pretty sure the visual styling needs to be adjusted to be more in line with the seven styleguide I think. (or with our maintenance pages)
Input needed from a UX specialist.
https://groups.drupal.org/node/283223

mgifford’s picture

@aspilicious I am happy to help provide some accessibility feedback. Not sure what is the best way to test this.

It's definitely important to be able to not throw up barriers for developers. I'm not at all worried about the background CSS image in this context. That's decorative as far as I can tell.

What are the areas of your concern?

I had assumed there was an issue queue to convert drupal_error.png to drupal_error.svg but haven't found it.

Aki Tendo’s picture

I was told in IRC by Creel (if I recall correctly) to go ahead and switch that out.

If you apply this patch from the Template Assertions child issue you'll get an assert failure on any page where Drupal is trying to assign two different elements on the same page the same ID. The views edit pages are guilty of this flaw.

Fabianx’s picture

Issue tags: -needs profiling

And here are the results for 50 nodes on the frontpage:

### FINAL REPORT

=== 8.0.x..8.0.x compared (5522cf93d0e0b..5522d1da94b86):

ct  : 700,473|700,473|0|0.0%
wt  : 956,270|960,705|4,435|0.5%
mu  : 36,550,032|36,550,032|0|0.0%
pmu : 38,876,496|38,876,496|0|0.0%

=== 8.0.x..issue-2408013--assertions--prod compared (5522cf93d0e0b..5522d28a205b6):

ct  : 700,473|683,438|-17,035|-2.4%
wt  : 956,270|942,705|-13,565|-1.4%
mu  : 36,550,032|36,546,808|-3,224|-0.0%
pmu : 38,876,496|38,879,464|2,968|0.0%

=== SUM: 8_0_x-summary..issue-2408013--assertions--prod-summary compared (5522d2d150347..5522d2d903d6f):

ct  : 70,051,947|68,348,447|-1,703,500|-2.4%
wt  : 97,633,908|95,550,114|-2,083,794|-2.1%
mu  : 3,655,603,944|3,655,114,400|-489,544|-0.0%
pmu : 3,888,299,936|3,888,419,624|119,688|0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+---------------------------------+------------+------------+------------+------------+------------+
| namespace                       |        min |        max |       mean |     median |       95th |
+---------------------------------+------------+------------+------------+------------+------------+
| Calls                           |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod |    683,438 |    688,085 |    683,484 |    683,438 |    683,438 |
| 8_0_x                           |    700,473 |    705,120 |    700,519 |    700,473 |    700,473 |
|                                 |            |            |            |            |            |
| Wall time                       |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod |    942,705 |  1,038,696 |    955,501 |    954,756 |    964,238 |
| 8_0_x                           |    960,705 |  1,209,839 |    976,339 |    972,259 |    988,549 |
|                                 |            |            |            |            |            |
| Memory usage                    |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod | 36,546,808 | 36,927,216 | 36,551,144 | 36,546,808 | 36,548,040 |
| 8_0_x                           | 36,549,944 | 37,048,688 | 36,556,039 | 36,550,032 | 36,550,032 |
|                                 |            |            |            |            |            |
| Peak memory usage               |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod | 38,878,392 | 39,278,488 | 38,884,196 | 38,879,464 | 38,880,856 |
| 8_0_x                           | 38,876,496 | 39,387,856 | 38,882,999 | 38,876,496 | 38,876,720 |
|                                 |            |            |            |            |            |
+---------------------------------+------------+------------+------------+------------+------------+

Without the xhprof overhead the assertions patch was 1.6% - 2% faster (measured with find-min-web.sh / index-perf.php 1000 times and xhprof off), therefore matching the xhprof result in percentage.

With assertions on it was 18% slower (and 5% more function calls - xhprof profiled) as an assert statement is essentially an 'eval', but that is a good tradeoff for the increased debuggability one gets.

I personally think the gain in performance is worth it as in the future we will rather have more context and tag merging and not less.

Crell’s picture

Aki: If I understand #147 correctly, you're describing an 18% loss in performance in "dev mode" (assertions on), but 2% gain in "prod mode" (assertions off)? Am I reading that correctly?

That's quite a spread, and it's debatable if that's a good trade-off. (Remember, performance during dev matters, too, and there will be lots of people that leave assertions on in production because they don't know any better.)

Aki Tendo’s picture

Aki: If I understand #147 correctly, you're describing an 18% loss in performance in "dev mode" (assertions on), but 2% gain in "prod mode" (assertions off)? Am I reading that correctly?

You're overlooking something very important -- that's a 2% boost with one optimization. I would expect further speed gains as more of the code changes over.

Assert mode should always be expected to run slower, sometimes very significantly slower since checks can (and will be) done on all sorts of things we don't normally check on.

That's quite a spread, and it's debatable if that's a good trade-off. (Remember, performance during dev matters, too, ...

While it does, turning assertions off while working on something that they are bogging down - like a large migration - is elementary.

... and there will be lots of people that leave assertions on in production because they don't know any better.)

That's already accounted for - The patch makes assertions off the default setting in the .htaccess file.

dawehner’s picture

Aki: If I understand #147 correctly, you're describing an 18% loss in performance in "dev mode" (assertions on), but 2% gain in "prod mode" (assertions off)? Am I reading that correctly?

If I undertand the issue correctly we would not turn them on by default but rather opt it in?

+ // Remember that raising an assertion while evaluating an assertion will
+ // cause a segmentation fault in the PHP engine. This assertion however
+ // cannot be fulfilled by a call under that circumstance UNLESS invoked
+ // by a handle function other than the one in FaultSetup.
+ assert('\\Drupal\\Component\\Fault\\Assertion::validCaller()', 'This class can only be used by other classes in the Fault Component');

I'd like to see us getting rid of that. Its a limitation, without knowing what people might need in the future, especially a limitation which is not disableable.

You're overlooking something very important -- that's a 2% boost with one optimization. I would expect further speed gains as more of the code changes over.

That is a good point.

+    assert('$cache_tags === array_filter($cache_tags, function($v){ return is_string($v); })',
+      'One or more invalid Cache Tags in passed array');

Just curious, is there a reason why we don't call out to the static method?

Wim Leers’s picture

You're overlooking something very important -- that's a 2% boost with one optimization. I would expect further speed gains as more of the code changes over.

Exactly.

If I undertand the issue correctly we would not turn them on by default but rather opt it in?

Yes!

Thanks for the patch, Aki, and for the profiling, Fabianx! IMO this is sufficient proof that assertions can be a valuable addition to core and help improve performance at the same time!


Here's an initial review of the patch.

  1. +++ b/core/lib/Drupal/Component/Fault/Assertion.php
    @@ -0,0 +1,109 @@
    +   * Test to see if a class is being constructed from the allowed scope.
    

    I think it might be better to do this in a follow-up issue: this change may need further discussion, the conversion of cache tag/context validation to assertions does not, and is sufficient to land the basic infrastructure.

  2. +++ b/core/lib/Drupal/Component/Fault/AssertionHandler.php
    @@ -0,0 +1,107 @@
    +    if (!$message && version_compare(PHP_VERSION, '5.4.8') === -1) {
    +      $message = 'Assertion description messages unavailable in PHP versions prior to 5.4.8';
    

    What are the consequences of this? Drupal 8's current minimum version is PHP 5.4.5.

    #2 says the description will be missing. How bad is that?

  3. +++ b/core/lib/Drupal/Component/Fault/FaultSetup.php
    @@ -0,0 +1,65 @@
    +   * Get the HTML for a quiet response.
    

    Do we even need this?

    We're going to disable assertion messages by default. A site should never ship with assertions enabled in production. The default matches what is needed for production. No need to babysit sites that enable assertions in production.

    Removing this would make the total set of changes smaller. We could still add this at a later time.

  4. +++ b/core/lib/Drupal/Component/Fault/sad-droop.svg
    

    Can we just remove this image? It's a nice touch, but likely controversial. Let's keep it simple.

  5. +++ b/core/lib/Drupal/Core/Cache/Cache.php
    @@ -37,8 +37,11 @@ public static function mergeContexts() {
    -    \Drupal::service('cache_contexts')->validateTokens($cache_contexts);
    +
    +    assert('\\Drupal::service(\'cache_contexts\')->validateTokens($cache_contexts)', 'One or more invalid tokens passed.');
    +
         sort($cache_contexts);
    +
    

    Let's restore the original whitespace.

  6. +++ b/core/lib/Drupal/Core/Cache/Cache.php
    @@ -66,8 +69,11 @@ public static function mergeTags() {
    -    static::validateTags($cache_tags);
    +
         sort($cache_tags);
    +
    +    assert('$cache_tags === array_filter($cache_tags, function($v){ return is_string($v); })',
    +      'One or more invalid Cache Tags in passed array');
    
    1. Let's do the sort() after the assertion, just like in ::mergeContexts(), and just like it was originally.
    2. Please put the assert statement on a single line (don't start a new line for the message).
    3. There's no need to capitalize "Cache Tags", "cache tags" will do.
    4. Let's restore the original whitespace.
    5. And finally: let's remove ::validateTags() :)
  7. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
    @@ -110,9 +110,7 @@ public function convertTokensToKeys(array $context_tokens) {
    +      assert ('in_array($context_id, $this->contexts)', 'Not a valid Context ID');
    
    1. Space between "assert" and opening parenthesis.
    2. Capitalization again: s/Context/context/
  8. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
    @@ -244,9 +242,7 @@ public function validateTokens(array $context_tokens = []) {
    +      assert ('is_string($context_token)', 'Cache contexts must be strings');
    

    Space again.

  9. +++ b/core/tests/Drupal/Tests/AssertionException.php
    @@ -0,0 +1,12 @@
    + * Defines Drupal\Tests\AssertionException.
    

    s/Defines Drupal\…/Contains \Drupal\…/

    Always "Contains \Drupal\…".

dawehner’s picture

What are the consequences of this? Drupal 8's current minimum version is PHP 5.4.5.
#2 says the description will be missing. How bad is that?

I think it would be fine if descriptions would be missing. I guess you still have the line of code available, which would make it feasible to fix the problem.

Aki Tendo’s picture

Issue summary: View changes

I'd like to see us getting rid of that. Its a limitation, without knowing what people might need in the future, especially a limitation which is not disableable.

I disagree. My experience is code getting used by 3rd parties in unexpected ways just leads to backwards compatibility nightmares that far outweigh the benefits. If they *really* need the functionality they can in the short term copypasta the code elsewhere and petition for the block to be removed with a concrete example as to why moving forward. That's much easier to deal with than having 15 modules using a public method 15 different ways all of which will get broken the moment a refactoring is attempted.

I've explained this multiple times at this point - public methods are a major source of compatibility problems. The more methods are public the more difficult something is to refactor, reorganize and otherwise improve. The benefits of "might be useful" are far outweighed by the ability to move the code forward without fear of breaking something.

The forward linking functions in FaultSetup should be considered an extension of the Handler class. The only reason they are in separate files is an optimization allowing for a lazy load. There is no reason to use them separately I an foresee. Furthermore, I plan to do further rewriting of this for 8.1 -- the class as seen here is a temporary placeholder only able to work with assertions. I do not want any code written in the interim that could get broken and the assertion of a valid caller is part of the process of seeing to it such code does not get written.

Just curious, is there a reason why we don't call out to the static method?

Probably should be ( Assertion::collectionOfStrings( $value ) ) and the conversion to that will get rid of the closure creation in an eval and probably speed up the assertion code considerably. I tend not to write a new method like that until I have two use cases for it though.

What are the consequences of this? Drupal 8's current minimum version is PHP 5.4.5.

Since the Assert aware unit tests use that string to verify the correct assertion was thrown those tests will fail on PHP 5.4.7 or prior. Otherwise there is no disruption. #2246775: Suggest 5.4.11 as minimum PHP version for Windows and MacOS if XCache is enabled wants the minimum to be 5.4.11 for different reasons, and I had filed a request to move to 5.4.8 in #2454071: [policy, no patch] Require PHP 5.4.8, so revisiting the minimum might be advisable. 5.4 end of life'ed at 5.4.30-something and since it is end of life perhaps set the minimum at that last version.

One other note - I've updated the issue summary.

dawehner’s picture

Yeah sure, its totally fair that you disagree here. I'd like to know what other people think about, maybe they don't care then its fine, or like that securing.

catch’s picture

The 2% performance improvement is enough reason for me to start using assertions prior to 8.0.0 where it helps there. The ct/wt reduction is 10-17ms which is very much worth having, and there'll be worse examples in the wild than 50 nodes.

How much plumbing we put in to support assertions I'm not sure about (and haven't caught up on that aspect of this issue since about two weeks ago - mostly afk atm).

Aki Tendo’s picture

Changes from Wim's code review completed, now testing assertions on. Notes

I think it might be better to do this in a follow-up issue: this change may need further discussion, the conversion of cache tag/context validation to assertions does not, and is sufficient to land the basic infrastructure.

At the moment the Fault System uses this to protect it's own handlers. I agree that a discussion concerning the use of this particular assertion needs to take place but for the moment please let it stay in to insure no one goes and tries to extend this component between now and 8.1

We're going to disable assertion messages by default. A site should never ship with assertions enabled in production. The default matches what is needed for production. No need to babysit sites that enable assertions in production.

Can we just remove this image? It's a nice touch, but likely controversial. Let's keep it simple.

Ok, removed.

And finally: let's remove ::validateTags() :)

Done. Let's see what breaks.

Other whitespace issues also addressed.

Aki Tendo’s picture

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
40.04 KB

Let's try this again, with some cold feet and leaving the ::validateTags() method well enough alone for now. Also restored cacheContexts->validateTokens to its original form and instead I'm having the assertion call a parallel method assertValidTokens on the object. The fewer changes, the less chance of regression.

Bojhan’s picture

Ieuww, what is this page? I dont really know what to review.

Aki Tendo’s picture

aspilicious’s picture

@bojhan this is a new error page for developers. We need someone to design it a bit so we can implement so that it will pass the design/ux gate

Aki Tendo’s picture

Issue summary: View changes

Ok, adding the following to the issue summary but also here.

Design Constraints.

A fault is a Fatal Error, uncaught exception or assert failure. The Fault Component is called upon to display the results of these failures to the user. Faults can come from anywhere in the system and can leave parts of the system unusable - for example if an assertion fails in the Drupal TwigExtension then Twig cannot be used since it will likely throw yet another error.

Fault handling is therefore tricky - One fault has already occurred so pain must be taken to insure that subsequent faults do not occur as they can really muck things up - even up to causing a segmentation fault in the PHP Engine itself.

Currently the Fault Class displays it's assertion message from an inline HEREDOC section. If desired this can be placed back into a PHP template. Since Twig can be destabilized by configuration or by the user changing the version of twig using composer it will not be used for this part of Drupal. External style sheets, javascript files and images may be referenced. In 8.0 the theme will not be configurable in any way, and it's unclear if such will ever be added. Ideally on a production site the general public will never see the fault system, nor will admins. Only developers should encounter it while creating new code, but if a bug escapes into a module it will be possible for the fault system to end up being seen.

The fault display screen needs to show the developer the following information:

  • The Type of fault. In 8.0 this will always be "Assertion Failure". In 8.1 it will be "Assertion Failure", "Uncaught Exception", or "Fatal Error".
  • Comment -- The programmer comment that is stored in the code along with the fault - or the fault message.
  • Class -- The class where the fault originated, linked to its documentation on api.drupal.org
  • Method -- As above for the method
  • Function -- If the fault comes out of a function this will be displayed instead of class and method.
  • Context -- The context of the error. Only Error messages show a context so this doesn't need to be worried about yet, but it's coming.
  • Stack Trace -- The stack trace of the fault. In Drupal 8.0 this will be from the var_dump function. In 8.1 it will be from the symfony/var-dumper dump() function.
Wim Leers’s picture

#162 looks awesome already, and a good 10 KB smaller too! :) Great!


(EDIT: I forgot a "not" in there… :P)

IMHO the design of the error page should NOT be a blocker. This will only ever be seen by developers. It can be changed/improved at a later time.

@Aki: would you like me to reroll your patch to fix all remaining nitpicks; to make the code comply with the Drupal coding standards?

Another partial review, to again make this patch simpler:

  1. +++ b/core/lib/Drupal/Component/Fault/BaseFaultHandler.php
    @@ -0,0 +1,361 @@
    +      openlog('Drupal 8', LOG_PERROR, LOG_USER);
    +      syslog(LOG_ERR, $entry);
    +      closelog();
    

    Hrm… not sure if we want this. Drupal has never been writing to syslog by default AFAIK.

    Can we remove this? Can be added later.

  2. +++ b/core/lib/Drupal/Component/Fault/BaseFaultHandler.php
    @@ -0,0 +1,361 @@
    +  protected function quietResponse() {
    

    I thought 158 removed this?

  3. +++ b/core/lib/Drupal/Component/Fault/FaultSetup.php
    @@ -0,0 +1,39 @@
    +      assert_options(ASSERT_CALLBACK, [__CLASS__, 'handleAssert']);
    +      assert_options(ASSERT_WARNING, 0);
    +      assert_options(ASSERT_BAIL, 1);
    

    The first line is clear.

    The second and third could use an explanation, so that people don't need to look up what this means/does. Please also include an explanation *why* we're changing the defaults. For example, the third line is changing the default to ensure that any assertion failure causes execution to be terminated: we want this to prevent more assertion failures/exceptions: the developer should fix this first.

  4. +++ b/core/lib/Drupal/Core/Cache/Cache.php
    @@ -110,6 +110,8 @@ public static function mergeMaxAges() {
    +   * @deprecated use assert('count($tags) === 0 || \\Drupal\\Component\\Fault\\Assertions::collectionOfStrings($tags)');
        */
       public static function validateTags(array $tags) {
    

    Shouldn't we just remove this method instead?

  5. +++ b/core/tests/Drupal/Tests/AssertionException.php
    @@ -0,0 +1,12 @@
    +/**
    + * On occassion we need to die immediately on assertion failure.
    + */
    

    Can you expand these docs to describe such occasions? It's not obvious to me.

    Also: s/occassion/occasion/ :)

Aki Tendo’s picture

1. The impression I got from the Logger class was the syslog was in use. The prefix codes it calls for are the same as the PHP predefined constants for logging. It can be removed but I wouldn't advise it - syslogging provides a final fallback point for error recovery in case the webserver is misconfigured.

2. There still should be a quiet response for when assertions are on and display_errors is off. That shouldn't happen, but people do make mistakes. The code for customizing this screen has been removed. Also, this code is being structured with its eventual takeover of Uncaught Exceptions and Fatal Errors in mind so this branch needs to stay since such fault displays will eventually occur, and the quiet response screen is better than a WSOD.

3. Additional comments added.

4. Cache::validateTags() is referenced from about a dozen locations in the code, and it's removal caused 827 test fails as seen at comment 158. Fixing this many errors introduces more changes than I would like for this patch and so I'd rather save the removal of that function for another issue ticket.

5. Documentation updated, but I missed correcting the typo for the moment. I'll get it in the next roll up, or you can if you wish to fix the remaining cs issues. Speaking of code standards, should we make any concerning the assert statement itself? The ones I had proposed were

  • Start of function assertions should come immediately after the parameter line, no space. A space must follow the last of the start assertions and the first line of operating code.
  • Comments related to assert statements should use Perl style comments ( # comment)
  • Mid function assert statements should be preceded and followed by a space.
  • End of function assert statements should not have a space between them and the return statements.
Aki Tendo’s picture

Issue summary: View changes

Added a note in the Issue summary that api.drupal.org still needs a section setup to host the assert explanations. I suggest the pattern of

http://api.drupal.org/faults/assertions/{assertion_string}

An assert statement pointing there would read

assert('false', 'api://assertion_string');

This needs to be set up so the patch can be modified to point at it and that pointing be tested.

aspilicious’s picture

+
+  # Assertions.
+  # By default PHP has these turned on. Production sites should turn these off.
+  # While assertions can be turned off at run time, we need to have this setting
+  # in place immediately to catch an assertions thrown before the settings file
+  # can be loaded.
+  php_value assert.active                   1
+  # To enable them, change 0 to 1. Recommended for dev sites.

This confuses me...
After reading the previous comments, I would suspect this being turned off by default.
Reading the comment tells me that it is turned on.

Aki Tendo’s picture

They are turned on by default, and Drupal will be turning them off by default - the version of .htaccess that will be shipped will read "php_value assert.active 0"

I keep leaving the .htaccess file set to 1 I need assertions on to do testing on the patch. The version that gets committed to 8.0.x will have the value set to 0.

Crell’s picture

To clarify, I wasn't saying 2% improvement wasn't worthwhile. I was comparing it with the 18% slowdown in dev. Suppose we then convert a bunch more stuff to assertions. Do both of those values scale? Vis, if we end up with a 10% improvement in production (very good) at the cost of a 90% slow-down for dev, I'm not convinced that's a good trade off. We want people to have assertions on for dev so that they can find bugs. If turning them on makes the site too slow to develop on comfortably, no one will turn them on. That's almost worse than the status quo. If that's not the scale of trade-off we're looking at, great; I don't know. That's why I'm asking, because 2 vs 18 set of my spider sense.

Aki Tendo’s picture

Dev isn't about getting things fast - it's about getting them right. If you don't like the incurred slow down develop your code with assertions turned off.

mpdonadio’s picture

Dev isn't about getting things fast - it's about getting them right. If you don't like the incurred slow down develop your code with assertions turned off.

I am not sure if this is a fair assumption. I think that a lot of developers will have local settings with assertions always on in their MAMP/WAMP/whatever. A slowdown in SimpleTest, which can run several minutes per test, would be brutal.

Crell’s picture

Yes. Correct > Fast in dev, up to a point. If it takes 5 seconds to load every page in dev despite a warm cache, even if it's fast in production I will still spit upon the grave of the developers who are putting me through such agony, then give up development and become a farmer. (Yes, I have worked on such sites, and yes, I have been tempted to go become a farmer.) Developers want speed, too.

We need some benchmarks to determine what the trade-off is going to be. Even some context-free micro-benchmarks could be helpful to figure out what we're looking at.

Aki Tendo’s picture

Optimization isn't really the point of Assertions though. The point is to make sure the code is working correctly by running checks that should only be possible to be failed with bad code.

That said, I think most of the 18% was my mistake of putting a closure into an assertion - which is essentially an eval. That's pretty high up there on the list of things that should not be done from an optimization standpoint, and there are better ways of accomplishing the same task.

Code optimization is a nice benefit to this, particularly in the case of how the cache system works, but it isn't the main goal. The main goal is improving system stability and making debugging easier.

If you don't want better code, run your tests and do your dev with assertions off and never use them. It's not hard.

Or realize how they are used and quit crying about the optimization hit when they are on. Turn them on only when a particularly weird error gets thrown, the system starts acting odd, or during the final test run. I would think this approach would be obvious. It also stops one of the more painful mistakes that can be made with assertions - treating them like conditionals or error statements.

And to be honest - I find assertions more useful for their trait of telling me when I'm reading the code what *shouldn't" be happening than actually having them turned on.

Fabianx’s picture

Yes, it was likely a worst case scenario. I am happy to do some more profiling - if we have a new patch without a closure?

Also:

Installing xhprof alone gives an overhead of almost 40%, still lots of developers develop with xdebug, xhprof and other tools enabled and it was not an issue.

We should test though how adding lots of assertions scales in terms of that. (What if we have 100 simple ones, e.g.)

Aki Tendo’s picture

The patch at #168 has the closure replaced with a call to collectionOfStrings(). At the moment it is the most optimized version of the patch. I'm doing other cache optimizations, but they'll be off in their own branch and issue patch.

Aki Tendo’s picture

Issue tags: +needs profiling

#2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts) most recent patch (#6) contains further cache optimizations built upon this patch to further aid profiling.

Aki Tendo’s picture

FileSize
55.6 KB

Last night's changes to head necessitate a re-roll.

Aki Tendo’s picture

FileSize
55.6 KB

110 patches to core have been made since the last time I tested this, so rediff'ing, retesting. No actual changes - I just want to make sure this still works. I still need this profiled please.

Bojhan’s picture

I am just wondering, can we use Seven CSS? This is independent from system failures, I assume.

Aki Tendo’s picture

I'm guessing yes - what do you mean by 'independent from system failures' though? The fault system is set off when a system failure occurs, specifically an exception, but its own code is independent of the rest of the system.

Aki Tendo’s picture

K, fixing that was rather painless. The mock cache_contexts_manager in the two failed tests in #181 must have it's assertValidTokens method set to return TRUE or else the assertion always fails.

Aki Tendo’s picture

Status: Needs work » Needs review
Aki Tendo’s picture

Ran some initial profiling of my own - I hope I got this right.

8.0.x as of this morning.

Server Software:        Apache/2.4.12
Server Hostname:        www.drupal8.dev
Server Port:            80

Document Path:          /
Document Length:        8616 bytes

Concurrency Level:      1
Time taken for tests:   506.273 seconds
Complete requests:      5000
Failed requests:        0
Total transferred:      49310000 bytes
HTML transferred:       43080000 bytes
Requests per second:    9.88 [#/sec] (mean)
Time per request:       101.255 [ms] (mean)
Time per request:       101.255 [ms] (mean, across all concurrent requests)
Transfer rate:          95.12 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:    88  101  18.1     99    1039
Waiting:       79   91  17.8     89    1013
Total:         88  101  18.1     99    1039

Percentage of the requests served within a certain time (ms)
  50%     99
  66%    102
  75%    104
  80%    105
  90%    109
  95%    112
  98%    119
  99%    125
 100%   1039 (longest request)

Patch applied, assertions on.

Concurrency Level:      1
Time taken for tests:   520.339 seconds
Complete requests:      5000
Failed requests:        0
Total transferred:      49310000 bytes
HTML transferred:       43080000 bytes
Requests per second:    9.61 [#/sec] (mean)
Time per request:       104.068 [ms] (mean)
Time per request:       104.068 [ms] (mean, across all concurrent requests)
Transfer rate:          92.54 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:    94  104  11.3    102     347
Waiting:       85   94  11.1     92     333
Total:         94  104  11.3    102     347

Percentage of the requests served within a certain time (ms)
  50%    102
  66%    105
  75%    107
  80%    108
  90%    112
  95%    116
  98%    123
  99%    129
 100%    347 (longest request)

Patch, Assert Off

Concurrency Level:      1
Time taken for tests:   484.743 seconds
Complete requests:      5000
Failed requests:        0
Total transferred:      49310000 bytes
HTML transferred:       43080000 bytes
Requests per second:    10.31 [#/sec] (mean)
Time per request:       96.949 [ms] (mean)
Time per request:       96.949 [ms] (mean, across all concurrent requests)
Transfer rate:          99.34 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:    87   97  10.3     95     303
Waiting:       78   87  10.1     85     291
Total:         87   97  10.3     95     303

Percentage of the requests served within a certain time (ms)
  50%     95
  66%     98
  75%    100
  80%    101
  90%    105
  95%    108
  98%    114
  99%    122
 100%    303 (longest request)

Comparison of the two most critical numbers...

                    8.0.x           Assert On       Assert Off
Requests/second:    9.88 [#/sec]    9.61 [#/sec]    10.31 [#/sec] (mean)
Time/request:       101.255 [ms]    104.068 [ms]    96.949 [ms] (mean)

Assertions on is 2% slower. Assertions off is 4% faster. Looks like having a closure in an assertion is a very big performance no-no.

Note that caching was disabled for these tests, forcing the cache tokens to be re-evaluated for all requests.

alexpott’s picture

@Bojhan #183 - the whole point is to not use theme system - the less dependencies the assertion system has the better.

Bojhan’s picture

@alexpott Yes, but the CSS is not really the theme system. It avoids having to write CSS specifically for this, and just implementing the classes.

Aki Tendo’s picture

@Bojhan - I was asked to reclassify this add on as a Component which means, in theory, it can be used by a PHP project completely unrelated to Drupal. For that reason it needs to carry it's own CSS files.

joelpittet’s picture

@Aki Tendo and @alexpott how about copying appropriate components from the Seven Style guide? As mentioned also by @aspilicious in #144

https://groups.drupal.org/node/283223

Aki Tendo’s picture

I see no reason not to.

Aki Tendo’s picture

Title: Fault System - for 8.0.x Assertions only, For 8.1.x this will will be a complete error handling system rewrite. » Fault System, Assert Handler, Cache-context and validate tags optimizations using assert to avoid unneeded calls in production.
Issue summary: View changes

Update title and summary.

Aki Tendo’s picture

FileSize
54.28 KB
20.64 KB

Changes for code standards.

Aki Tendo’s picture

FileSize
54.3 KB

Interdiff still relevant - only index.php changed between this patch and 194

Aki Tendo’s picture

Status: Needs work » Needs review
Bojhan’s picture

I suggest we reuse the HTML structure and classes of the maintenance page without directly using the twig template, but a bastardised version of it (just the HMTL).

Aki Tendo’s picture

Sounds good to me. Where's the maintenance page in the system?

Wim Leers’s picture

Assertions on is 2% slower. Assertions off is 4% faster.

We should confirm that on another machine, but if that is correct, that's awesome!

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
54.51 KB

Missed a use statement in the index, causing every simpletest using the web system to fail.

Aki Tendo’s picture

Any progress on the profiling?

Bojhan’s picture

@Aki Teno IF you put the site in maintaince mode you should see it.

Aki Tendo’s picture

K. Well then, updated. Rather than port the stylesheets in I looked up the computed values for the visible elements and wrote a new inline sheet that holds that data.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
54.88 KB
3.87 KB

Trying again.

dawehner’s picture

2% are certainly something not bad for the frontpage. Given that we are fast by default, this patch should actually ship with a assertions turned off, right?

One thing which I was curious before is though the following: Why are other people not using it? For example symfony, they seem to rely on exception all over the place,
why exactly have they decided to go with that and not assertions? This seems to be quite a major knowledge for that issue if you ask me.

  1. +++ b/core/lib/Drupal/Component/Fault/Assertion.php
    @@ -0,0 +1,121 @@
    +class Assertion {
    +  /**
    

    Nitpick: Our code style adds a new line here.

  2. +++ b/core/lib/Drupal/Component/Fault/AssertionHandler.php
    @@ -0,0 +1,108 @@
    +  /**
    +   * Implements BaseFaultHandler::composeLogEntry.
    +   */
    ...
    +  /**
    +   * Implements BaseFaultHandler::verboseResponse.
    +   */
    

    Should be {@inheritdoc}

  3. +++ b/core/lib/Drupal/Component/Fault/BaseFaultHandler.php
    @@ -0,0 +1,374 @@
    +    // Find Drupal root from here - we don't know if DRUPAL_ROOT is defined.
    +    $this->root = dirname(dirname(dirname(dirname(dirname(__DIR__)))));
    

    It is odd to have that as part of the component system, even technically we still rely on the way how Drupal is structured. Note: A component should be usable outside of Drupal itself as it is.

  4. +++ b/core/lib/Drupal/Component/Fault/BaseFaultHandler.php
    @@ -0,0 +1,374 @@
    +  /**
    +   * Parse out the trace to find the class and method location of the fault.
    +   */
    +  protected function parseTrace(array $trace, $file, $line) {
    ...
    +   */
    +  protected function getApiPageForMethod($file, $class, $method) {
    ...
    +  protected function getApiPageForFunction($file, $function) {
    

    Another nitpick: We should have documentation here for those parameters.

  5. +++ b/core/lib/Drupal/Core/Cache/CacheContextsManager.php
    @@ -271,4 +271,16 @@ public function validateTokens(array $context_tokens = []) {
    +  public function assertValidTokens(array $context_tokens = []) {
    +    try {
    +      $this->validateTokens($context_tokens);
    +    } catch ( \LogicException $e) {
    +      return FALSE;
    +    }
    

    I'd expected that the exception would be somehow takei nto account, given that it contains potentially valuabel information during debug time? Maybe \LogicException is a special case, so it maybe needs some documentation.

  6. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
    @@ -210,7 +210,7 @@ public function setMultiple(array $items) {
    +      assert('count($item[\'tags\']) === 0 || \\Drupal\\Component\\Fault\\Assertion::collectionOfStrings($item[\'tags\'])', 'One or more invalid Cache Tags in passed array');
    
    +++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
    @@ -107,7 +107,7 @@ protected function prepareItem($cache, $allow_invalid) {
    +    assert('count($tags) === 0 || \\Drupal\\Component\\Fault\\Assertion::collectionOfStrings($tags)', 'One or more invalid Cache Tags in passed array');
    

    What is the resaon for this count() statement?

  7. +++ b/core/modules/simpletest/src/AssertionTestingTrait.php
    @@ -0,0 +1,51 @@
    +trait AssertionTestingTrait {
    
    +++ b/core/tests/Drupal/Tests/AssertionTestingTrait.php
    @@ -0,0 +1,55 @@
    +trait AssertionTestingTrait {
    

    Can we somehow reuse the same code?

  8. +++ b/core/modules/simpletest/src/AssertionTestingTrait.php
    @@ -0,0 +1,51 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function assertAssertionNotRaised() {
    

    @inheritdoc on protected methods feels wrong.

  9. +++ b/core/modules/simpletest/src/AssertionTestingTrait.php
    @@ -0,0 +1,51 @@
    +  protected function assertAssertionsRaised() {
    +    $this->assertIdentical(func_get_args(), $this->assertionsRaised, 'Expected Assertions Raised.');
    

    It seems to be that the much better DX would be to explicit pass along the expected raised assertions as array. func_get_args() method is IMHO always hard to understand.

Aki Tendo’s picture

One thing which I was curious before is though the following: Why are other people not using it?

I don't know. If I had to guess, it's the mistaken belief that Test Driven Design covers all eventualities. It does not. Tell me this, can you write a unit test for code that will be written 3 years from now to make sure it will follow the API guidelines? It's not possible.

As you know better than I, Drupal has a lot of moving parts. Unit tests against classes and packages work well, but system wide tests are a bear to write to begin with. Also, a lot of the newer developers don't unit test.

Assert is part of a methodology known as "design by contract" As such it is idea for monitoring the interactions between disparate blocks of code. It is not a replacement for Test Driven Design but a supplement. As a rule of thumb, only public methods should need to assert anything, and most assertions will exist to examine the input and output of functions. Until we make the shift to PHP 7 assert is the only way to test a function parameter to insure it is an expected primitive type, and it is the only way to test a function return to insure it is of the correct type. Once PHP 7 becomes the baseline assert's role will be reduced, but not eliminated.

Assert is also more critical for Drupal since we have more beginning programmers than other projects, and it can catch the configuration missteps they make very quickly.

Finally I did a search of the vendor libraries. Guzzle uses the assert function 7 times in 3 files, so it's use is not unknown in other projects out there.

It is odd to have that as part of the component system, even technically we still rely on the way how Drupal is structured. Note: A component should be usable outside of Drupal itself as it is.

It was Alex Pott's call to move this from core to components, and I did not remove the Drupal specific functionality at that time. If that functionality is removed then why not fall back to BooBoo or Whoops? The ability to scan the API and return a Drupal specific explanation is pretty key to the concept of the module, but I didn't complain at the time the request was made because I didn't fully understand the dichotomy between Core and Components. Now that I do I feel this probably should be core despite it's lack of interaction with the rest of the system because the information look ups are, and will always be, Drupal specific.

I'd expected that the exception would be somehow taken into account, given that it contains potentially valuabel information during debug time? Maybe \LogicException is a special case, so it maybe needs some documentation.

Knowing the triggering string would be useful, but unfortunately assert is looking for a boolean response and there's no way to pass back that information back up the call stack.

What is the resaon for this count() statement?

The assertion::collectionOfStrings will return false if there are 0 objects in the collection - logically how can it be a collection of something if it's empty? ValidateTags, the method being replaced here, does tolerate a collection of 0, so we need to check these separately for BC. I did it this way to avoid future confusion - all of the collectionOf methods expect to see at least 1 item in the collection and all of them are the correct type.

Can we somehow reuse the same code?

As much code as possible is being reused - both those traits use a third trait, BaseAssertionTestingTrait, which contains their common methodology. Simpletest and PHPUnit assert functions are not the same, forcing these two classes to exist.

@inheritdoc on protected methods feels wrong.

All assert functions in both PHPUnit and Simpletest are protected. Both of these functions implement an abstract method in BaseAssertionTestingTrait

It seems to be that the much better DX would be to explicit pass along the expected raised assertions as array. func_get_args() method is IMHO always hard to understand.

I'm of the opposite opinion. This construct is going to see a lot more use in the future because of the implementation of the splat operator in PHP 5.6. If that was our baseline the function would start

protected function assertAssertionsRaised( ...$assertions )

So you're likely to see a lot more of this pattern in the future. To the outside world though which is easier?

$this->assertAssertionsRaised( 'InvalidTag', 'illegalCaller');

or

$this->assertAssertionsRaised( ['InvalidTag', 'illegalCaller']);

I'd rather not have to stop and remember to pass an array and just overload the function, and while func_get_args is ugly it allows for a gentler transfer once PHP 5.6 is the lowest common denominator (Drupal 9 at the earliest I'd imagine)

All other changes called out have been applied, see interdiff.

At the end of the day though this is a stylistic choice, one of the two needs to be picked and enforced across the system for consistency. I'm not particularly married to either one, but it's something others need to chime in on.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
57.25 KB

CacheContextManager mocks must have a assertValidTokens mock method that returns true, otherwise the assertion always fails. Tests adjusted.

Aki Tendo’s picture

Ok, green again. Still need profiling done.

Fabianx’s picture

Assigned: Aki Tendo » Fabianx

Profiling this now.

Aki Tendo’s picture

(posted to wrong issue ticket)

Fabianx’s picture

Quick re-roll

Aki Tendo’s picture

If it fails the most likely reason is a new test that has a cache_contexts_manager mock. This line is needed on those mocks.

$cache_contexts_manager->method('assertValidTokens')->willReturn(TRUE);

Reason - the old method, validateTokens, throws an exception and returns null, so the mock doesn't need to do anything to simulate that. The new method using an assertion needs a TRUE return to satisfy the assertion and stop it from failing.

Moving forward this will be true of any mock that needs to return a value to an assertion, slightly increasing the complexity of test writing.

Fabianx’s picture

Assigned: Fabianx » Unassigned
Issue tags: -needs profiling

50 nodes, front page, render cache = off, page cache = off

### FINAL REPORT

=== 8.0.x..8.0.x compared (555c9455ee6a2..555c98fde68fe):

ct  : 613,486|613,486|0|0.0%
wt  : 855,255|849,755|-5,500|-0.6%
mu  : 35,848,552|35,848,552|0|0.0%
pmu : 38,108,032|38,108,032|0|0.0%

=== 8.0.x..issue-2408013--assertions--prod compared (555c9455ee6a2..555c995a98d3d):

ct  : 613,486|593,458|-20,028|-3.3%
wt  : 855,255|830,578|-24,677|-2.9%
mu  : 35,848,552|35,844,904|-3,648|-0.0%
pmu : 38,108,032|38,106,024|-2,008|-0.0%

=== SUM: 8_0_x-summary..issue-2408013--assertions--prod-summary compared (555c99c29ed3b..555c99cb0e921):

ct  : 61,353,235|59,350,435|-2,002,800|-3.3%
wt  : 86,455,928|84,647,477|-1,808,451|-2.1%
mu  : 3,585,408,296|3,585,067,616|-340,680|-0.0%
pmu : 3,811,333,656|3,811,200,024|-133,632|-0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+---------------------------------+------------+------------+------------+------------+------------+
| namespace                       |        min |        max |       mean |     median |       95th |
+---------------------------------+------------+------------+------------+------------+------------+
| Calls                           |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod |    593,458 |    598,093 |    593,504 |    593,458 |    593,458 |
| 8_0_x                           |    613,486 |    618,121 |    613,532 |    613,486 |    613,486 |
|                                 |            |            |            |            |            |
| Wall time                       |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod |    830,578 |    926,828 |    846,475 |    845,356 |    853,793 |
| 8_0_x                           |    849,755 |    948,203 |    864,559 |    863,985 |    872,952 |
|                                 |            |            |            |            |            |
| Memory usage                    |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod | 35,844,832 | 36,376,608 | 35,850,676 | 35,844,904 | 35,844,904 |
| 8_0_x                           | 35,848,552 | 36,380,800 | 35,854,083 | 35,848,552 | 35,848,840 |
|                                 |            |            |            |            |            |
| Peak memory usage               |            |            |            |            |            |
|                                 |            |            |            |            |            |
| issue-2408013--assertions--prod | 38,105,984 | 38,648,048 | 38,112,000 | 38,106,024 | 38,106,024 |
| 8_0_x                           | 38,107,152 | 38,648,184 | 38,113,337 | 38,108,032 | 38,108,032 |
|                                 |            |            |            |            |            |
+---------------------------------+------------+------------+------------+------------+------------+

I did not hit the nicest baseline this time, but even with that, we are still around 2% faster on average, which matches the profiling done before.

Aki Tendo’s picture

Excellent. Next piece I can't do alone is to get api.drupal.org set up to host fault explanations. Who would I contact regarding that? And what path do we wish to map to?

Crell’s picture

During DrupalCon I put a call out on Twitter for input on the "why is it so unused?" question. I didn't get a great deal of feedback, and some googling turned up mostly what we already know: "Hey it's there, it's got some issues, but it might be nifty." So no big red flags on why we shouldn't use assertions, but no clear trend toward using them, either.

At this point, I think we need an official green light from the framework managers. Alex Pott sounded fairly positive above, but now that we have some numbers I'd ask him to officially stamp this issue (and note it in the issue summary) before it goes further, to avoid wasted work.

Assuming it does get green lit, I'd recommend moving forward with good messages (as Alex notes above) without links to Drupal.org. Those should be decoupled, because changes to d.o being a blocker for a core issue is a great way to ensure the core issue never happens. :-) A new issue should get spun up in the Drupal.org queue for figuring out the on-site logistics; whenever that gets resolved a second pass here could add the links to the site.

Aki Tendo’s picture

Issue summary: View changes
Aki Tendo’s picture

I hashed out with Alex and jhodgdon in IRC where the assertion messages will actually live. As far as killing the message functionality - at that point there's very little reason not to just keep the assertions but cut everything but the test and test assisting code and set the assert bail flag to true because that will have most of the same effect.

The first and principle goal of this patch is to make simplified, understandable, error messages available. Most of the time that involves explaining what is going on inside Drupal. If we aren't going to do that, why do any of this? Whoops and BooBoo exist. We don't need to be making yet another error handling system just because we can unless we need functionality they don't, and cannot provide. That functionality is system specific look up of error messages.

Anyway, the final path for the fault messages will be in the developing/api section of drupal.org - specifically

http://www.drupal.org/developiong/api/faults/assertions/8/assertionLabel.

The fault handler will be written to look to see if the assertion message starts with @. If it does it will pull the first word or line from the message.

If it's a number it presumes that's a node id on drupal.org. This will make linking error messages to tickets easily done.
If it's a string starting with http then that link will be posted as a "reference link"
If it's any other string the system will write the link for now in the manner seen above. Come 8.1 there will be exceptions and errors as appropriate to those types.

The code that figures out what API pages are relevant is already working and doesn't need changing.

I would prefer to see this placed back in the Core namespace since it is not an exportable component, nor was it ever meant to be, but I'll wait until that change is green lighted before doing it.

Fabianx’s picture

I very much like #221 as a proposal, that makes it independent from any d.org deployment and also gives most flexibility.

Aki Tendo’s picture

The root faults landing page - 1st draft:
https://www.drupal.org/node/2492185
Needs the alias https://www.drupal.org/developing/api/faults

The assertions landing page - 1st draft
https://www.drupal.org/node/2492225
Needs the alias: https://www.drupal.org/developing/api/faults/assertions

The assertion of valid caller - 1st draft
https://www.drupal.org/node/2492355
Needs the alias https://www.drupal.org/developing/api/faults/invalidCaller/8

Aki Tendo’s picture

As Crell recommends I have decided to halt all further development on this until I have a green light to proceed.

Fabianx’s picture

Assigned: Unassigned » alexpott

Assigning to alexpott for official framework manager review.

hass’s picture

I have a question about this task.

Google Analytics allows tracking of exceptions, but D8 core just shows me a blank white page and I cannot add any JS code to this page. How can we implement this feature to allow me adding an exception tracker to exceptions?

Crell’s picture

hass: For tracking exceptions that bubble all the way to index.php, you don't. That's too low level for Drupal to have any way to give you a hook point. However, that's an extremely small percentage of exceptions.

For most exceptions, they'll get caught by the Kernel and passed to the Exception event listeners, one of which will handle HTML-based exceptions. See DefaultExceptoinHtmlSubscriber and related. (Please open a new issue if you have further questions, it's off topic for this thread.)

Aki Tendo’s picture

No, don't make a new issue ticket - one already exists

https://www.drupal.org/node/2444089

This issue is a single part of that larger issue which will deal with uncaught exceptions and handling them correctly rather than allowing a white screen of doom from occurring.

Crell’s picture

Aki: That's a completely different question than what I told hass to make a new issue for, which is HTML header injection in case of non-200 HTML responses. :-)

Aki Tendo’s picture

Ah. Ok (got crossed up there).

Still waiting for orders on how to proceed.

catch’s picture

I still feel the same as I did in #155 - very +1 on using assertions, a bit ambivalent about adding the fault system prior to 8.0.0 (but at least partly that's about finding time to review it properly).

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs framework manager review

Like @catch I'm +1 to using assertions. I still have concerns about the heaviness of the current fault system. More to maintain and more to go wrong. I too have concerns about reviewing the patch properly.

I applied the patch and added assert(1 == 2, 'yay'); to index.php. I got a long message saying:
The Assert statement was passed a non-string value. Whatever expression was sent to it will be evaluated regardless of whether assert functions are turned on or off. In order to preserve system efficiency it is imperative that you encapsulate the expression in a string to be evaluated by assert rather than passing an expression to it.
So looking what I did - I was like yep I pass a boolean - ok I'll pass a string so I did:

$a = 1 == 2;
assert((string) $a, 'yay');

... same message :) It transpires that assert('1 == 2', 'yay'); is what I actually should be doing.

Wrt to linking in to developer documentation on Drupal.org I'm completely torn. I can see how it will help make Drupal easier to developer for and will allow us to improve documentation without changing core code. Doing the links to d.o is the reason this patch is so large. As @Aki Tendo noted if we remove this then there's

very little reason not to just keep the assertions but cut everything but the test and test assisting code and set the assert bail flag to true because that will have most of the same effect.

So this is a tough call. I've talked about this with @catch - he was in favour of splitting this into two issues - one to start using assertions. And a second issue to add the fancy handler and linking to d.o - that way hopefully both patches will be easier to review and both issues will get the quality reviews needed. I agree with @catch. If people add assertion messages in between patch 1 and 2 then they can still link to pages on d.o in messages. And we can convert them to use the new system once patch 2 lands.

Aki Tendo’s picture

... same message :) It transpires that assert('1 == 2', 'yay'); is what I actually should be doing.

Yeah, Thank the PHP dev team for that cute problem. It's been around since 4.0 so I don't think it's gonna get fixed and it might be the a reason other projects have been reluctant to use assert.

Anyway, let's go ahead with the plan to split the handler logic off. That will leave this ticket with the following to evaluate.

  1. .htaccess settings We need to add a section to the htaccess file that turns assertions off, and I would recommend having a line in there to advise to the programmer that if they turn them back on they should probably set assert.bail to 1 as well to be safe.
  2. Double Check Guzzle This is the only 3rd party library we are using that has assert statements - checking to make sure it's not being misused as a control structure would be prudent.
  3. Drupal\Component\Fault\Assertion This assisting class has 3 methods. The first in the file, assertValidCaller, will it ever be used? I still feel it's worth having as a supplement to marking items as @internal, but if we don't intend to make use of it it should be dropped. The other two methods are sufficiently utilitarian as to not need heavy checking. The other classes in this namespace are going to be deferred to a later patch so there's another question - does this belong here? Core code has assertions that will be calling collectionOfStrings. My current thought is this class needs to be reassigned to the Drupal\Tests\ namespace. While not a unit test itself, it is a testing mechanism and won't be invoked in production.
  4. Drupal\Tests\ToStringMock Does PHP Unit have a way to do this I'm overlooking? I know it can mock interfaces. This seems incredibly low level not to exist somewhere out there somehow.
  5. Drupal\Tests\BaseAssertionTestingTrait This class has two children, one for PHP Unit and one for simpletest. The simpletest one might not be necessary given dawehner's Kernel PHP unit test patch.
  6. Cache Optimizations And their related tests.

With the deferment of the handler to 8.1 that becomes the scope of this ticket - which is to provide a means to test assert statements with PHPUnit and simpletest when necessary, and then provide some cache optimizations.

Proceed with this?

alexpott’s picture

Thanks @Aki Tendo for a clear plan it looks to me - and thank you for sticking with this. I think in the long run we'll end up with a great system that helps developers and reduces fragility whilst developing on Drupal 8.

Some specific points:

  • #223.2 I've checked Guzzle - it's use of assertions are all in documentation?!? as far as can see.
  • #223.3 I think we should add things as we need. Less to review and less to ensure works.
  • #223.4 None of the test infrastructure we depend on seems to have anything like this.
  • #223.5 Simpletest and the current KernelTestBase is going to go before 8.0.0 so both children will be necessary
Aki Tendo’s picture

Title: Fault System, Assert Handler, Cache-context and validate tags optimizations using assert to avoid unneeded calls in production. » Adding Assertions to Drupal - Test Tools.
Assigned: alexpott » Aki Tendo
Issue summary: View changes
Issue tags: -Needs accessibility review

Updating the issue summary to bring it into line with the goals outlined in 233.

I've checked Guzzle - it's use of assertions are all in documentation?!?

Documented or not, assert statements are in there. This is the return of a find in files command in sublime text.

/drupal8/core/vendor/guzzlehttp/ringphp/docs/client_handlers.rst:
  151      $mock = new MockHandler(['status' => 200]);
  152      $response = $mock([]);
  153:     assert(200 == $response['status']);
  154:     assert([] == $response['headers']);
  155  
  156  Implementing Handlers

/drupal8/core/vendor/guzzlehttp/ringphp/docs/client_middleware.rst:
   91      ]);
   92  
   93:     assert($response['headers']['X-Header'] == 'hello!');
   94  
   95  Built-In Middleware

/drupal8/core/vendor/guzzlehttp/ringphp/docs/testing.rst:
   54      ]);
   55  
   56:     assert(200 == $response['status']);
   57  
   58      $response = $handler([
   ..
   62      ]);
   63  
   64:     assert(404 == $response['status']);
   65  
   66  After requests have been sent, you can get a list of the requests as they
   ..
   71      $received = Server::received();
   72  
   73:     assert('GET' == $received[0]['http_method']);
   74:     assert('HEAD' == $received[1]['http_method']);
   75  

The calls are done in non-eval fashion, but since they are simply boolean compares they won't create a performance loss, though the maintainer of the package might want to change those checks into eval'able strings just to be sure.

4 None of the test infrastructure we depend on seems to have anything like this.

True, but I added it because I'm writing tests that need it and its so low level that I can foresee it being reused a lot. My use case is the unit test for Assertion::collectionsOfStrings() needs to use a toString mock. That method in turn is used by the assertions tied to the cache optimizations. The existing unit tests of the cache library still pass but the change does allow an object to be used as a cache token so long as that object has a __toString method that returns a valid token string.

Aki Tendo’s picture

Issue summary: View changes

Noticed some typos in the issue summary.

twistor’s picture

@Aki Tendo, what @alexpott was saying about Guzzle is that the assertions are only used in documentation, not in actual code. Note the docs/client_handlers.rst path.

@alexpott, Those assertions are used when building docs with Sphinx as a way to validate the example code is still correct. Kind of nifty actually.

Aki Tendo’s picture

I need help sorting out what some permanent namespaces are going to be, as well as what some things are going to be named.

I'm pretty sure this is going to be a component - the Assertion component. So namespace

\Drupal\Component\Assertion

First question, are test assisting materials allowed to be placed here. If so that will give us this file library.

\Drupal\Component\Assertion\Assertion
\Drupal\Component\Assertion\AssertionUnitTestTrait
\Drupal\Component\Assertion\AssertionException

That would comprise the component part which as I understand components should be movable out of Drupal and back into the outside PHP world without incident. The code above provides a way to write unit tests that can test an assert statement cleanly in both PHPUnit and Simpletest

Speaking of which, the only unit test of this component is

\Drupal\Tests\Component\Assertion\AssertionTest

Which runs the Assertion library through it's paces.

The rest of the patch deals with the cache modifications and, since there are so few of them, the Template engine asserts that propelled me into this project to begin with.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
20.72 KB

Ok, the patch is rebuilt with the new namespaces and all changes requested. The cache optimization materials will be handled in a child ticket and separate patch - their role in proof of concept now concluded.

No interdiff - way too many changes to make one comprehensible.

JeroenT’s picture

Status: Needs work » Needs review

@Aki Tendo,

I haven't checked the patch, but is it the right one? Because it's almost 40kb smaller than the patch in #215.

Aki Tendo’s picture

Yes. The difference comes from the decision to defer the Assert Handling functionality to 8.1 and to move the cache optimizations off to a child ticket.

Test failed cause I forgot the test suite now requires the @group to be declared. No code differences between the patches, just changing the comment text.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
19.64 KB

Wrong namespace in test.

JeroenT’s picture

@Aki Tendo,

Keep up the good work then! ;-)

Aki Tendo’s picture

^ Thanks.

Ok, patch is green and needs RBTC. I know of one typo in the comment text. I'm starting with the cache optimizations now.

Aki Tendo’s picture

Typo corrections in comment text, no code changes.

Aki Tendo’s picture

During cache hardening and optimization I found a bad use statement in the simple test class.

Crell’s picture

  1. +++ b/.htaccess
    @@ -28,13 +28,29 @@ DirectoryIndex index.php index.html index.htm
    +  # Now override PHP settings that cannot be changed at runtime. See
    +  # sites/default/default.settings.php and
    +  # Drupal\Core\DrupalKernel::bootEnvironment() for settings that can be
    +  # changed at runtime.
    +  php_flag session.auto_start               off
       php_flag session.auto_start               off
    

    If I'm reading the patch correctly, this adds a redundant session.auto_start line.

  2. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,104 @@
    + * This is a static function hive for use by the assert statement on the more
    

    I don't think "hive" is a word we've used anywhere else. Amsuing, but probably not the best word choice here unless you're a Nick Cage fan. Maybe "collection"?

  3. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,104 @@
    + * complicated assertions we wish to make which are of use to the general PHP
    + * community. Complicated Assertions that deal with Drupal's idiosyncracies
    + * should be attached to the relevant class.
    

    I like the rest of this comment, though.

  4. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,104 @@
    +   * @param string $property
    +   *   Property to test for, usually an interface or type.
    +   * @param array|Traversable $traversable
    +   *   An array or traversable object.
    +   */
    +  public static function allMembersAre($property, $traversable) {
    

    I would just go ahead and call this $type, not $property. "property" suggests property of an object, eg $foo->property, and it took me a while to realize that's not what was intended here.

    Additionally, there's a lot of duplication of possible type names here. To be more forward looking, I would go only with the type names used in PHP 7. Eg, if PHP 7 allows both int and integer, do the same, but if it allows only one of them use just that. This is an opportunity to start training people on the types we'll be using in the future; let's take it. (We can allow a few others like numeric, that PHP 7 doesn't have but not those that are simple aliases.)

  5. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,104 @@
    +    foreach ($traversable as $obj) {
    +      switch ($property) {
    +        case 'array':
    +          if (!is_array($obj)) {
    +            return FALSE;
    +          }
    +          break;
    

    Part of me is dying inside seeing a big long switch statement. I was about to suggest a set of array_filter() directives instead, but then remembered that only works for arrays, not other traversables, because PHP is stupid.

    I'd love to see this cleaned up a bit though, if possible.

  6. +++ b/core/lib/Drupal/Component/Assertion/PHPUnitTestingTrait.php
    @@ -0,0 +1,31 @@
    + * Methods for testing the internal PHP assert function in PHP Unit.
    

    PHPUnit is one word.

Aki Tendo’s picture

If I'm reading the patch correctly, this adds a redundant session.auto_start line.

Oops. Fixed.

I don't think "hive" is a word we've used anywhere else. Amsuing, but probably not the best word choice here unless you're a Nick Cage fan. Maybe "collection"?

Done.

I like the rest of this comment, though.

Thanks.

I would just go ahead and call this $type, not $property.

Type isn't exactly right either because it's not always an object type. It can be a test to see if all the array members aren't empty which is a condition or whether the variable can be expressed as a string through being a string natively or having the __toString method. I'm open to suggestions on a different term here, but type is just as confusing as property.

I did expand the comment text for the parameter to attempt to clarify this further.

Additionally, there's a lot of duplication of possible type names here. To be more forward looking, I would go only with the type names used in PHP 7.

That would be float and integer. Numeric isn't a datatype test. The test is based on PHP's collection of is_* functions, so there actually is as is_double(), is_real(), is_long() functions in PHP at the moment. I was going to shorten the code some by using

if (function_exists('is_' . $property)) {
  if (!call_user_func('is_' . $property, $obj)) {
    return FALSE;
  }
}
else 
...

But given how these asserts live in MASSIVE loops like the cache system I became worried about the performance hit of using call_user_func, which is slower than directly invoking the function. I went with the wordier but hopefully speedier (for the computer) implementation.

Part of me is dying inside seeing a big long switch statement. I was about to suggest a set of array_filter() directives instead, but then remembered that only works for arrays, not other traversables, because PHP is stupid.

array_filter might be faster for the arrays, but it might be slower. Consider a cache collection with 100 tokens and the 2nd one is invalid - there's no need to check the next 98 cause the assertion has failed and we can return - whereas array_filter must be concluded and then the arrays need to be checked for equal count. But from a code reading point of view that would be much nicer.

PHPUnit is one word.

Corrected.

One other change this go round -- is_object added. I found that the cache system has functions that take arguments of object collections with no common type so the only assertion I can do for the moment is with is object. I would strongly recommend creating an interface for cache-ability if only to label objects the cache system can capture.

alexpott’s picture

Status: Needs review » Needs work
  1. This patch is definitely nice and focussed on providing the assert statement functionality and testing it in Drupal - nice.
  2. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # PHP enables assert statements by default. Drupal uses this statement to
    +  # perform validations that are only necessary during module and theme
    +  # development, and that slow the system down. Hence the Drupal default is off.
    +  php_value assert.active                   0
    +
    +  # Assert statements are used to validate the cache. To prevent invalid cache
    +  # entries from being written during development you need to turn assert bail
    +  # on. You need to do this even if you disabled caching as Drupal writes caches
    +  # even when it isn't reading from them.
    +  php_value assert.bail                     0
    

    Need to change web.config for the IIS version of this. Also "Drupal uses assert statements to" pr "Drupal uses them to". There are a couple of lines over 80 characters here.

  3. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # The above two settings can be changed at runtime using the assert_options
    +  # function. We set them here since assert statements can appear in the code
    

    "The above two settings can be changed ar runtime using assert_options(). Set them here to enable assert statements before \Drupal\Core\DrupalKernel::bootEnvironment() runs."

  4. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # See Assertions in sites/example.settings.local.php
    

    @see sites/example.settings.local.php

  5. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # Now override PHP settings that cannot be changed at runtime. See
    

    The "now" is redundant.

  6. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,113 @@
    + * PHP community. Complicated Assertions that deal with Drupal's idiosyncracies
    

    Mis-spelt idiosyncrasies and unnecessary capital A on assertion.

  7. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,113 @@
    +   *    - Primitive type such as string or integer
    

    Missing fullstop

  8. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,113 @@
    +   * @param array|Traversable $traversable
    +   *   An array or traversable object.
    +   */
    +  public static function allMembersAre($property, $traversable) {
    

    Missing @return documentation.

  9. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,113 @@
    +        case 'callable':
    +          if (!is_callable($obj)) {
    +            return FALSE;
    +          }
    +        case 'not-empty':
    +          if (empty($obj)) {
    +            return FALSE;
    +          }
    +          break;
    

    If the lack of a break here is intentional can it be documented.

  10. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,24 @@
    + * Assert Fail Exception.
    

    This should be a one line that says what it does.

  11. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,24 @@
    + * In development we die immediately on assertion failure. In most of the tests
    + * we allow the test to continue despite the failure to check to see if the code
    + * remains (relatively) stable despite the failure - this is done because assert
    + * statements, unlike conditionals, can be turned off, and we need to test code
    + * behavior in that situation.
    + *
    + * Some assertions however assert conditions that *will* bring the code to an
    + * immediate and ungraceful halt soon after the assertion if they are failed.
    + * In order to test those failures we'll need to tell the assert handler in the
    + * AssertionTestingTrait to convert the assert statement into an exception so
    + * that it can be handled by PHPUnit.
    

    "In development we die immediately..." that's a bit of a dark start. There are several lines longer than 80 chars. Also this all feels very verbose. Also much of it is repeated below. You can use @see to avoid unnecessary repetition in documentation. I would argue that all assertion that fail should cause PHPUnit to fail since that it how it currently handles all errors.

  12. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,146 @@
    + * By default, both convert assert failures to exceptions stopping the code in
    

    Is this true - it does not seem that it is converting assertions to exceptions by default anymore. Actually the stated behaviour of assertions in tests makes sense to me - why are we throwing an exception when an assertion occurs in a test?

  13. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,146 @@
    + * its tracks.  These methods allow the tester to log the last assertion
    

    Extra spaces.

  14. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,146 @@
    +  /**
    +   * Errors Expected.
    +   *
    +   * This flag escapes the check for unaccounted assertions. If an error occurs
    +   * any assertions raised before it will be unaccounted for, so we can't check
    +   * for them during tear down.
    +   *
    +   * @var bool
    +   */
    +  protected $thereWillBeErrors = FALSE;
    

    This appears never to be set to True - is it obsolete?

  15. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,194 @@
    +use ArrayObject;
    

    Not used

  16. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTestTraitTest.php
    @@ -0,0 +1,83 @@
    +   * @expectedException Drupal\Component\Assertion\AssertionException
    

    Missing a leading slash

  17. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * Drupal uses the assert statement to perform validations that are only
    

    "Drupal uses assert statements..."

  18. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * necessary during module and theme development, and that slow the system down.
    + * By default the statements are turned off in the .htaccess file.  Here we turn
    

    Double space after . and lines are longer than 80 chars.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
20.87 KB
13.66 KB

1. Thank you.
2. I don't know how to adjust the web config and even if I did I don't have an environment set up to test it. I do have coworkers with IIS that can help me with this Monday. Text changes made.
3-8. Done
9. That was a mistake. I'm surprised the test passed despite that.
10. I don't understand.
11-13. I've re-written these blocks to clarify what's going on and try to be less verbose.
14. This was an artifact of the assertValidCaller method. When testing exception throws on classes using that assertion the assertion would be thrown since the test class isn't in the namespace the assertion looks for, then the error happens. The teardown method sees the assertion was thrown and fails the test since it wasn't accounted for - but there's no opportunity from the moment the error is thrown to check for the assertion.

The situation where both an assertion raises and an exception throws should be incredibly rare and is indicative of a method that's trying to do too many things, so I've simply removed this code.

15. Yes it is starting at line 31. Or do you mean I'm not to use the use statement to bring a class from the root namespace into the current?

16-18. Fixed.

alexpott’s picture

10. The first line of a class description is supposed to be a one-line sentence that makes sense and tells you what it does. Funnily enough there is no explicit mention on https://www.drupal.org/coding-standards/docs#classes but the one line summary is the same for methods, functions and anything else.
15. I meant in core/tests/Drupal/Tests/Component/Assertion/AssertionTestTraitTest.php - ArrayObject is not used in that file although it has a use statement.

I'm yet to actually review the changes made in #252.

Aki Tendo’s picture

FileSize
20.84 KB
1.66 KB

Ok, changes for the above applied as well. Thanks.

Aki Tendo’s picture

FileSize
20.27 KB
3.07 KB

Looked back over this after the weekend, found some grammar corrections to make. Some redundant comments removed. There was an error in the code sample in the comments of the Assertion class. No code changes.

Aki Tendo’s picture

FileSize
23.64 KB
3.68 KB

At Alex's request, api documentation added.

Aki Tendo’s picture

FileSize
3.93 KB

Revised and condensed entry. Interdiff only, no new code.

Aki Tendo’s picture

FileSize
3.01 KB

Even further condensed at jhodgdon's request in IRC.

Aki Tendo’s picture

Status: Needs work » Needs review

Last patch was an interdiff and should not have been tested.

Aki Tendo’s picture

This may warrant it's own ticket - let me know. These are my recommendations regarding the formatting of assertions.

Assertion Code Standards

Precondition assertions

These are assertions about the parameters of a function. An example from the Core\Theme\Registry class

  protected function processExtension(array &$cache, $name, $type, $theme, $path) {
    assert('static::assertValidProcessExtensionCacheArgument($cache)');
    assert('is_string($name)');
    assert('in_array($type, ["module", "theme_engine", "base_theme_engine", "theme", "base_theme"])');
    assert('is_string($theme)');
    assert('is_string($path)');

    $result = array();

1. There is to be no new line between the function start line and the first assertion.
2. One parameter - one assertion.
3. Assertions are to be in parameter order.
4. A new line goes between the last precondition assertion and the body of the function UNLESS the function is only one line to begin with - example:

  protected function getPath($module) {
    assert('is_string($module)');
    return drupal_get_path('module', $module);
  }

Postcondition assertions

These are assertions concerning the return of a function.

    }
    assert('\\Drupal\\Component\\Assertion\\Assertion::allMembersAre( \'\\Drupal\\Core\\Theme\\ThemeNegotiatorInterface\', $this->sortedNegotiators)');
    return $this->sortedNegotiators;
  }

1. A blank line should precede the first assertion unless it's just a closing brace from a control structure.
2. There should be no space between the assertion and the return.
3. Try to insure functions with a postcondition assertion do not do have any early returns.

Other assertions have no special formatting.

jhodgdon’s picture

Status: Needs review » Needs work

Um, where's the new patch to review?

Aki Tendo’s picture

FileSize
21.98 KB

Just to avoid confusion, a re-roll.

Aki Tendo’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Whew!

I had some time today and reviewed the API documentation and topics in about half of the patch.... I ran out of time but the docs need some work along the same lines in the rest of the patch....

Interesting project though!

  1. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # on. You need to do this even if you disabled caching as Drupal writes caches
    +  # even when it isn't reading from them.
    +  php_value assert.bail                     0
    

    Minor suggestion: I would change "as" to ", because"

    Also I don't think I would say "drupal writes caches", but rather "Drupal writes to the cache".

  2. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # The above two settings can be changed at runtime using assert_options()
    +  # Set them here to enable assert statements before
    +  # Drupal\Core\DrupalKernel::bootEnvironment() runs.
    +  # See sites/example.settings.local.php
    +  #
    

    Normally we want documentation to preceed what it is documenting, so maybe this block should be moved up?

  3. +++ b/.htaccess
    @@ -28,13 +28,28 @@ DirectoryIndex index.php index.html index.htm
    +  # Override PHP settings that cannot be changed at runtime. See
    +  # sites/default/default.settings.php and
    +  # Drupal\Core\DrupalKernel::bootEnvironment() for settings that can be
    +  # changed at runtime.
       php_flag session.auto_start               off
    

    You've moved this comment block into the IfModule mod_php5.c area. But I think it also applies to all of the other settings in the other sections, so I think it should not have been moved here.

    Perhaps the right fix is to leave it where it was, but change the first line to say something like:

    Most of the following settings cannot be changed at runtime.

    Alternatively, you could put the new assert settings into their own block before this comment block, which might be better anyway?

  4. +++ b/core/core.api.php
    @@ -57,6 +57,7 @@
    + * - @link assertions Assert Statements @endlink
    

    So... "Assert Statements" is not really an unambiguous title. We use Assert statements in a lot of PHPUnit and Simpletest tests, for instance. Is it possible to make the link text here less ambiguous?

  5. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * @defgroup assertions Assert Statements
    

    Would also want to disambiguate this topic title.

  6. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * Use of the Assert statement in Drupal.
    

    The actual statement looks like

    assert()

    So I don't think it should be put into this line as "the Assert statement". It should be instead "the assert() statement".

  7. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * in the code it appears at. They are tested using PHP's internal
    + * The @link http://php.net/manual/en/function.assert.php assert @endlink
    + * statement. If an assertion is ever false it indicates an error in the
    + * configuration files or elsewhere in the code.
    

    Take "The" out of this second line.

    Also do not link specifically to the English manual. You can make a generic link by taking the /en/ out of the URL, and it will redirect people to docs in their own language (test and make sure).

    Also we normally want () after PHP function names in docs.

    And use TRUE/FALSE not true/false for PHP docs.

    Why call out configuration files specifically as the source of the error here?

  8. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * When using assert keep the following in mind.
    

    assert()

    Also end this line in a : before a list.

  9. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * - Assertions are disabled by default in production and enabled in dev.
    

    in dev -> on development sites

    But... um... how is this "dev" default done by Drupal? Are you saying they *should* be disabled by default etc.?

  10. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * - Because of the above they can't be used as control structures.
    

    I think I would say something a bit different here, something like:

    PHP allows assert() to be completely disabled; when assert() is disabled, the code within assert() is not executed. So, when using assert(), be careful not to assume that the code inside it will be executed.

  11. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * - Always use a string argument for your condition test to minimize overhead
    + *   when assertions are turned off.
    

    Ummm... what's a condition test? Oh. I think you mean to say:

    Always pass a string into assert() (which will be evaluated as PHP) rather than an evaluated value, to minimize...

  12. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + *   when using them to work with any data that may be tainted.
    

    tainted => unsanitized

  13. +++ b/core/core.api.php
    @@ -983,6 +984,33 @@
    + * @see https://www.drupal.org/node/2492225
    

    Normally in these topics, we write out something like:

    See https://whatever for more information on ...

  14. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    + * Collection of methods to assist the assert statement.
    

    assert()

  15. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    + * This is a static function collection for use by the assert statement on the
    

    assert()

  16. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    + * This is a static function collection for use by the assert statement on the
    + * more complicated assertions we wish to make which are of use to the general
    + * PHP community. Complicated assertions that deal with Drupal's idiosyncrasies
    + * should be attached to the relevant class.
    

    This first sentence is a bit convoluted and probably shouldn't be talking about "wish".

    Also you need a comma before "which" if you want to use this sentence as it is.

    And I would not really put "idiosyncrasies" in this!

    And What would the "relevant class" be here ?

    So... I was going to suggest modified documentation here but I think that the one-line description of this class is really sufficient and this paragraph doesn't really add much. It already says they're helpers.

  17. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   * Tests a collection to insure all members share the stated property.
    

    I would avoid the use of "test" in this class. We're talking about "assert", so say something like "Asserts that all members of a collection share a property."

  18. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   *   assert('Drupal\\Component\\Assertion\\Assertion::allMembersAre(\'string\', $array');
    

    Normally rather than escaping single quotes, our coding standards suggest using double-quotes on the outer string... would that work here? Otherwise maybe double-quotes on the inner strings?

  19. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   *   Property to test for one of:
    

    This needs punctuation, like "Property to test for; one of:"

  20. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   *    - Class or interface.
    

    Shouldn't this be two separate items?

  21. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   *    - Class or interface.
    +   *    - Primitive type such as string or integer.
    +   *    - Status, such as not being empty, callable, or castable to string.
    +   * @param array|Traversable $traversable
    

    List is indented too far. Should line up under the P of Property.

    And this last line needs more explanation. How would I indicate one of these statuses?

    The Class one should probably say "The name of a class with the namespace, but not starting with \" rather than just Class too?

  22. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +   * @return bool
    +   */
    +  public static function allMembersAre($property, $traversable) {
    

    @return needs documentation line.

  23. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +    foreach ($traversable as $obj) {
    

    Local variable names should not be abbreviated. So this should be $object not $obj... is it always an object though? It seems it isn't, so probably it should be $item?

  24. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'array':
    

    This option is not documented in the @param

  25. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        // As above but allows objects which implment __toString(). Unless
    

    implement is misspelled

    which -> that

  26. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        // you have a specific reason to test for strings specifically (Such
    

    Such should not be capitalized

  27. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'stringable':
    

    This option is not documented in the @param.

  28. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'int':
    +        case 'integer':
    +        case 'long':
    +          if (!is_int($obj)) {
    

    Really, you want to allow this much flexibility?

  29. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'double':
    +        case 'float':
    +        case 'real':
    +          if (!is_float($obj)) {
    +            return FALSE;
    

    again, this much flexibility? Why not just document what to pass in to get this test?

  30. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'callable':
    

    This option is not documented in the @param

  31. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'not-empty':
    

    not documented in @param.

    I am not going to put in comments like this any more but the @param needs a real list of what the values can be and what they mean. You shouldn't have to read the code.

  32. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        // Reminder - is_numeric() returns true on strings with a value that
    

    TRUE not true

  33. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        // Use this sparingly if at all, it's usually best to call for a
    +        // specific object class or interface.
    

    This is a comma splice

  34. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,122 @@
    +        case 'object':
    

    Just a note that 'object' and 'array' are not technically "primitive" types, as suggested by the @param docs.

  35. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * Exception to be thrown on assert raise when required.
    

    ummmm... not sure what this means?

  36. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * Assertion aware tests (those which implement PHPUnitTestingTrait or
    

    Assertion-aware needs hyphen

  37. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * (relatively) stable despite the failure. This is done because assert
    

    assert()

  38. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * Some assertions test conditions that *will* bring the code to a halt soon
    

    We don't normally use * for emphasis in docs

  39. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * Some assertions test conditions that *will* bring the code to a halt soon
    + * after the assertion when they fail. If PHPUnit or simpletest is allowed to
    

    maybe this should say something like "will cause major problems if left to continue" ?

    Also "simpletest" is ... ???

  40. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,28 @@
    + * catch these errors any record that the assertion was thrown will be lost.
    

    needs comma after "errors".

  41. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * Methods for unit testing code which uses the PHP assert function.
    

    assert()

    and I think you've referred to it elsewhere in the patch as an assert() *statement*; here it's called the assert() function. Pick one and be consistent.

  42. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * By default both PHPUnit and Simpletest will stop when an assertion fails.
    

    So... This is very confusing. Both PHPUnit and Simpletest have other things called "assertions". You need to rewrite this so that it makes clear that this is talking about the PHP assert() statement, not those types of assertions. To make it even more confusing, the PHPUnit and Simpletest base test classes, I believe, have assert() methods. So writing this to avoid confusion is tricky.

  43. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * that directly checks if an assertion passes or fails when it should.
    

    Again... an "assertion"?

  44. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * Since Drupal has two testing systems at the moment, this trait has two
    + * children.
    

    Traits don't have children. And you probably don't need to document that the trait is used in two or more different classes.

  45. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * it would in production with assert statements turned off. Tests
    + * implementing this trait check this log to see if the expected assert
    + * failures did indeed occur and then removes them from the log. If
    

    This sentence has some verb agreement problems.

  46. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * Tests can also fail if an assert raise is expected but does not occur.
    

    What's an assert raise?

  47. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * set the dieOnRaise flag to TRUE to cause an AssertionException to be
    

    Is dieOnRaise a member variable? if so, prefix it with the class/trait name (with the namespace).

    Also AssertException needs namespace. We always use fully-qualified class names in docs.

  48. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    + * public function tearDown() {
    + *   $this->stopAssertionHandling();
    + *   parent::setUp();
    + * }
    + * @endcode
    

    should probably be parent::tearDown()

    Also is that right that you want to stop assertions *before* calling the parent tear down function? Seems like the stop should be the last thing to call? If not, it may be useful to explain why not.

  49. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    +trait BaseTestingTrait {
    

    Odd name for this trait, if it is about assertion handling. Shouldn't it have the word Assertion in the trait name?

  50. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    +  protected $dieOnRaise = FALSE;
    

    There's this word "raise" again. Is that standard terminology? I do not see it anywhere on

    http://us1.php.net/manual/en/function.assert.php

  51. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    +   * Collections of captured assertions.
    

    Collections? Wouldn't it just be one collection?

    Should maybe say:

    The captured assertions.

  52. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    +   * @var array
    

    Normally we don't like @var array and prefer it to be @var \Some\Thing\Collected[]

  53. +++ b/core/lib/Drupal/Component/Assertion/BaseTestingTrait.php
    @@ -0,0 +1,138 @@
    +  protected $assertionsRaised = [];
    +
    +
    +  /**
    +   * Callback handler for assert raises during testing.
    

    Extra blank line here

  54. This is where I ran out of time, sorry! Really, read through the docs and make them smaller and clearer...
jhodgdon’s picture

One other suggestion on the patch: Once you have @defgroup ... defined, you can use @ingroup on the Trait and any other related classes, so that they'll be listed on the Topic page on api.drupal.org. See
https://www.drupal.org/node/1354#defgroup

Aki Tendo’s picture

1-3. Revisions done should address these points.

5. Changed to PHP Runtime Assert Statements. Also, when double checking this please help me by making sure I consistently refer to all uses of the assert() statement as a "runtime assertion" in the documentation.

6. Done.

7. First four points addressed. Module configuration files, particularly the services directive, can cause the service container to improperly set up a system service and cause unexplained crashes throughout the system. The line has been changed to specificy module and theme configuration files. Errors in user provided configurations should be handled by normal control structures and exceptions.

8. Fixed.

9. Fixed. Reference to sites/example.local.settings.php added.

10. Merger of lines should clarify. Also keep in mind that PHP does execute code that is within assert() in defiance of the convetion of all other programming languages - hence the advisement to give it a string argument!

11-15. Fixed.

16. Rewritten.

17. Fixed

18. No. This is a very bad idea. This is your suggestion:

assert("Drupal\\Component\\Assertion\\Assertion::allMembersAre('string', $array)");

If $array is an array this string is evaled:

Drupal\\Component\\Assertion\\Assertion::allMembersAre('string', Array)

Which always fails the assertion. An object will crash the program. Worst of all, if the array isn't sanitized and a string is passed in that string can rewrite the implicit eval() allowing the system to be hacked. So no, code standards can't be followed here.

Double quotes on the inner strings is possible - but I am (justifiably) paranoid around eval(). Personally I hate PHP making assert() work this way. One of my hopes is given the size of the Drupal project - if we start extensively using it then maybe - MAYBE - the PHP team will fix the damn statement to make it behave like it does in all other programming languages.

19-41. Fixed.

42. Again, throught the docs the term "runtime assertion" refers to the assert() statement as distinct from the assertions in PHPUnit and Simpletest.

42-47. Revised text for clarity addressing these issues.

48. Correction made, explanation given.

49. Name changed.

50. Well, the term "throw" is used for Exceptions. I suppose "trigger" could be used in the vein of "trigger_error()". I believe 'raise' is proper terminology but I'm self trained so I won't vouch for it. I'm using the term to avoid using "throw" because this isn't an exception - it can't be caught.

51-52. Fixed

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
22.26 KB

I hate renaming things. Something always breaks when renaming.

Crell’s picture

At least from a little naive Googling, "raise" seems to be the term for assertions in Ruby, Python, and Lua. The PHP documentation page manages to avoid using a verb. :-)

I agree that "throw" should be reserved for exceptions, where that is the syntax used.

Aki Tendo’s picture

Status: Needs review » Needs work

That's probably where I picked it up from - I used to use Prototype framework, and that was created by the Rails team.

@all Hold off on code reviews for a bit, I'm doing a rewrite to break up that monster case statement in the Assertion class.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
31.37 KB
22.48 KB

Rewrote the Assertion Tools, breaking up the AllMembersAre function into discrete chunks.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
31.38 KB

Why that static error didn't pitch locally I dunno.

Crell’s picture

A couple of minor mostly non-blocking nits. I fully expect the next patch posted to be RTBCable. Nice work, Aki!

  1. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,368 @@
    +  /**
    +   * Assert a key exists in every member array.
    +   *
    +   * Drupal has several data structure arrays that require certain keys be set.
    +   * You can overload this function to specify a list of required keys. All
    +   * of the keys must be set for this method to return TRUE
    +   *
    +   * @param mixed $traversable
    +   *
    +   * @return bool
    +   */
    +  public static function assertAllHaveKey() {
    

    This docblock could benefit from an example. It's tricky to understand what you'd do with this method without reading the code.

  2. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,368 @@
    +  public static function assertAllFloat($traversable) {
    +    if (static::assertTraversable($traversable)) {
    +      foreach ($traversable as $member) {
    +        if (!is_float($member)) {
    +          return FALSE;
    +        }
    +      }
    +      return TRUE;
    +    }
    +    return FALSE;
    +  }
    

    Pedantic note: Most of these assertAll*() methods could get centralized to a single method that takes $traversable and a callable, and then you'd just call that from each of these methods with 'is_float', 'is_callable', etc. as the second param. Not a requirement, but it would save quite a few lines of redundant code.

  3. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,368 @@
    +      foreach ($traversable as $member) {
    +        if (!is_string($member)) {
    +          return FALSE;
    +        }
    +
    +        if ($case_sensitive) {
    +          if (!strstr($member, $pattern)) {
    +            return FALSE;
    +          }
    +        }
    +        elseif (!stristr($member, $pattern)) {
    +          return FALSE;
    +        }
    +      }
    

    Micro-optimization: Moving the case sensitive check outside the foreach reduces the number of times $case_sensitive needs to be checked, which makes the code probably not much measurably faster. :-) (Unless you want to be a pedant about it, which I generally do.)

  4. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,368 @@
    +  /**
    +   * Assert all members are objects.
    +   *
    +   * You can overload this function. The first argument is required and is the
    +   * collection of objects to test.  The second and subsequent arguments are
    +   * the classes and interfaces to test for. The method returns TRUE if all
    +   * members of the traversable are indeed objects, and if you pass in any
    +   * classes and interfaces the method will return TRUE if the object has
    +   * at least one of those interfaces.
    +   *
    +   * Note that if it is important enough to your code that a collection be
    +   * tested to be a collection of objects then it is important enough to give
    +   * all of those objects a common interface and test for just that interface.
    +   * This method is written to allow for other tests to be written to minimize
    +   * potential disruption, but they should be used sparingly if at all.
    +   */
    +  public static function assertAllObjects() {
    

    Good description! But a code sample would help here anyway. (Any time you have a pseudo-variadic function, a code sample helps.)

  5. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +trait BaseAssertionTestingTrait {
    

    This or something like it would be worth contributing upstream to PHPUnit itself, IMO.

    (Totally not a blocker for this issue to do so, but it would be a nice follow-up to allow PHPUnit an @expectsAssertion or something so that we could use that in the future instead.)

Aki Tendo’s picture

Thanks Crell.

1. Example provided. I also added commentary that actually using this assertion is going to be rare because if you need to be specific about the keys, you usually need to be specific about their data types, and such is outside the scope of this class. I go on to cite a function that does this - but it's in a child patch of this. Will this be ok?

2. Doing that would require using call_user_func() repeatedly. Doing this from the scope of an eval() statement (which runtime assertions unfortunately are) will have severe performance problems. The is_* functions left out are those I don't see being used frequently enough to justify - specifically is_bool() and is_resource().

Speaking of the necessity to use string on assert() - as of PHP 7 this will no longer be the case. Also, PHP 7 unifies assert failures to the exception mechanism :D

https://wiki.php.net/rfc/expectations

I only learned of this over the weekend, but I guess it means the code needs to be checked against PHP 7 to see if it behaves.

3. Done.

4. Done

5. PHP 7 makes this unnecessary since PHPUnit at that point will be able to @expectedException AssertionException to test for them. Indeed, the traits part of this patch are now only needed while PHP 5.x is supported.

In addition to adjustments for the above, the code has been sent through PHP Code Sniffer for code compliance. I did drop a lot of param arguments on the Assertion tools methods because it would lead to a lot of redundancy - many of them take any argument and return bool. The exceptions have their parameters detailed but every method in the class returns bool.

Fabianx’s picture

#276: The last interdiff seems incomplete, maybe the comparison target was the last commit instead of the last patch you provided?

Thanks!

Crell’s picture

I think #276 has the wrong interdiff, based on the comment.

Re point 2: No, since it's a known number of parameters it would only need to be $callable($var), not call_user_func(). Really, call_user_func() should never be used as it offers no advantages over $function() but is twice as slow. (The difference between foo() and $foo() is barely measurable.) call_user_func_array() is still needed until 5.6 when variadics were introduced. For some admittedly rather dated micro-benchmarks, see http://www.garfieldtech.com/blog/benchmarking-magic (I should rerun those one of these days.)

Re point 5: Huzzah on PHP 7! But even PHPUnit is still going to support 5.6 for quite some time. Again, not a blocker here, just an encouragement to talk to Sebastian about it and see if he'd be interested.

Aki Tendo’s picture

@Fabian - yeah, the interdiff is just the latest set of changes, not all changes since the last time I provided an interdiff. Sorry.

@Crell
2. Cool. I thought $callable($var) would only work if $callable was a closure. Still, I prefer things as they stand since it creates self documenting code. Which reads smoother?


Assertion::assertAllStrings($arg);
Assertion::assertAllAre('is_string', $arg);

And also, I'd like to keep my tongue in cheek unit test joke.

$this->assertFalse(Assertion::assertAllFloat(['Titanic']));

And that is the test that strings aren't floats.

As to PHPUnit.. I'd like to get this patch out so leave it alone for now, but I could write an upstream patch that adds an assert callback to PHP Unit that throws an AssertionException just as PHP 7 would do. That way the tests can look for the expected exception regardless of PHP version. That sound good?

Crell’s picture

Aki: No no, I was suggesting assertAllStrings() would just subcall to a protected assertAll($arg, 'is_string') method internally. No change to the public API, which is fine. Again, not a blocker to commit just me being pedantic. :-) (Which means you'd keep the joke.)

Re PHPUnit: Yes, no impact on this patch. This is just me doing my job as a collaboration advocate, trying to convince you to do work upstream in PHPUnit that we can leverage later. :-) No changes needed here at this time.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
33.26 KB
1.35 KB

Mentioning a core class by full name in the comments of a component causes the Drupal component test to fail. So the reference is changed to something the test won't pick up.

alexpott’s picture

I think I've given enough "framework manager" opinion on this issue. I think asserts are a great tool for Drupal 8 to be using and I'm happy that the implementation has been slimmed down to the bare essentials.

Aki Tendo’s picture

Thanks Alex.

Crell's refactor suggestion incorporated. Hopefully this is final draft.

jhodgdon’s picture

Status: Needs review » Needs work

This is definitely a LOT better than the last time I looked at this patch!

I read through it all and have a few comments, mostly things that need to be fixed in the documentation.

  1. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * theme configuration files (User provided configuration files should be
    + * tested with normal control structures and exception throws, not assert()
    + * statements).
    + *
    

    User should not be capitalized here... or else the preceding files should end that sentence with . and then put the . at the end of the () remark inside the . since it will be a complete sentence.

    Also user-provided should be hyphenated.

    Also... I don't think "normal control structures and exception throws" are really "tests"? I would use a different word here rather than "tested". Maybe "verified"?

  2. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * Since unit tests also use the term "assertion" to refer to test conditions
    + * the term "runtime assertion" will be used to refer specifically to uses of
    + * the assert() statement.
    + *
    

    Maybe a comma at the end of the first line would help?

  3. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * The Drupal project has adopted runtime assertions to monitor interactions
    

    has adopted => uses ?

  4. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * between the components, modules and themes that make up the project during
    

    The Drupal project writing standards say we should use serial commas. So needs a comma after "modules" on this line.

  5. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * between the components, modules and themes that make up the project during
    + * development. They supplement unit tests by being able to check the validity
    + * of code that hasn't been written yet or that has no unit tests of its own.
    

    When I read this, I went "... um, make up the project during development? How does the project makeup change after development?".

    I think maybe this needs rewording to make it clearer that "during develpment" is referring to "monitoring interactions" and not "making up the project".

    Also... really the purpose is to "monitor interactions"? ???

  6. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * development. They supplement unit tests by being able to check the validity
    + * of code that hasn't been written yet or that has no unit tests of its own.
    + * These checks slow the system down so they are disabled in production.
    

    How can you check the validity of code that hasn't been written yet? This is odd wording. Really, you're asserting that input to some function or that a relationship to some data has some format, and future code that also calls into the assert-decorated code will also be checked... I guess?

  7. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + *   development so they can't be used as control structures. Use exceptions
    

    comma after development.

  8. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + * - Always pass a single quoted string into assert() to minimize overhead
    + *   when assertions are turned off.
    + * - Assertions are parsed the same way as the eval() statement and therefore
    

    If assertions are turned off, won't PHP just completely ignore the line in its compile phase? How does using a single quoted string actually minimize overhead? I don't get this.

  9. +++ b/core/core.api.php
    @@ -983,6 +984,45 @@
    + *   have the same security implications as that statement. Use caution when
    + *   using them to work with any data that may be unsanitized.
    + *
    

    How about:

    Use caution when using them with data that may be unsanitized.

    for the last sentence here?

  10. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +  /**
    +   * Assert argument can be traversed with foreach();.
    +   */
    +  public static function assertTraversable($traversable) {
    +    return is_array($traversable) || $traversable instanceof Traversable;
    

    See
    https://www.drupal.org/node/1354#functions

    First lines should start with a verb like "Asserts" not "Assert".

    This applies to all the methods in this class.

    That ; at the end should be removed too, and probably foreach doesn't need a () after it (it's not a function).

    I also think that this particular one would be clearer if it was written:

    Asserts that the argument can be traversed with foreach.

    Also you have not documented the function parameter. Must do so.

    And the return value too.

  11. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Inspect each member of a traversable with a callback function.
    

    inspect -> inspects
    (see previous comment)

    But actually.. this doesn't tell what the function does. Please document it.

  12. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * @param callable $callable
    +   * @param object|array $traversable
    +   *
    

    This is not acceptable documentation for parameters -- needs descriptions.

    See https://www.drupal.org/node/1354#param

  13. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * @return
    +   *   FALSE if any member returns FALSE, else return TRUE.
    +   */
    

    @return should have a type on it (@return bool).

    Also this needs more information on what "any member returns" means.

    And it also apparently (looking at the code) returns FALSE if $traversable is not a traversable, and this is not documented.

  14. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all members are strings.
    

    Assert -> Asserts that

    This also needs @param and @return docs.

  15. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all members are strings or objects with magic __toString() method.
    

    Assert -> Asserts that

    This will make it longer than 80 character line however, so it will need to be rewritten. You could probably remove the word "magic".

    Also needs @param and @return docs.

  16. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all members are associative arrays.
    

    Assert -> Asserts that

    Also needs @param and @return docs.

  17. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * This is the superset for PHP arrays. The other array tests are more
    +   * specific than this.
    +   */
    

    what does this mean, "superset"? It seems to mean that it is less specific of a test?

    Probably should also have @see references to the other methods that you are referring to, and from those to this one?

  18. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert array is strict.
    

    Assert -> Asserts that

    This also needs @param and @return docs.

  19. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * A 'strict' array is an 0 indexed array in the classic sense found in
    ...
    +    foreach (array_keys($array) as $key) {
    

    an 0 indexed => a 0-indexed

    This is kind of unclear though... looking at the code, it seems to be checking that the keys are increasing numbers, right?

  20. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * because we want to return false if the passed argument is not an array
    

    false -> FALSE

  21. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all members are strict arrays. See previous method.
    

    "See previous method" is not OK here.

    What you want to do is separate this out into two lines.

    a) Assert that ...

    b) then leave a blank line

    c) Then use @see to make a reference to the other method.

  22. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert a key exists in every member array.
    

    Assert -> Asserts that

  23. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Drupal has several data structure arrays that require certain keys be set.
    +   * You can overload this function to specify a list of required keys. All
    +   * of the keys must be set for this method to return TRUE.
    +   *
    +   * As an example, this assertion tests for the keys of a theme registry.
    +   *
    +   * @code
    +   *   assert('Drupal\\Component\\Assertion\\Assertion::assertAllHaveKey(
    +   *     $arrayToTest, "type", "theme path", "function", "template", "variables", "render element", "preprocess functions")');
    +   * @endcode
    +   *
    

    You could probably write this more concisely by doing it as @param $array and then @param ... and documenting that you can pass in one or more keys.

  24. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * This particular runtime assertion will often be temporary - if a method
    +   * requires certain keys to be present it will usually be specific about the
    +   * data types of those keys. Testing for this is outside the scope of a
    +   * general purpose toolset. The Drupal theme registry class provides one
    +   * example of such a specific test. Such tests are either bound to the
    

    huh?

  25. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * general purpose toolset. The Drupal theme registry class provides one
    +   * example of such a specific test. Such tests are either bound to the
    +   * object that uses them, or are collected into one assertion set for the
    

    Use the actual class name instead of "The Drupal theme registry class". With namespace. And maybe a method on it. Then someone could actually look at it.

  26. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all members are integer values.
    

    OK I'm going to stop saying "Assert => Asserts that" and "Needs @param and @return docs". Applies to all the rest of the methods here.

  27. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Assert all member are strings which contain the specified string.
    

    member -> members
    which -> that

  28. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * @param mixed $traversable
    +   *   String[] with all members containing the needle needed to pass.
    +   * @param bool $case_sensitive
    

    You are mixing up the @param and the @return here. I would put just something like "Array of strings to test" as this doc. Also do not use String[] -- this does not exist.

  29. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * @param bool $case_sensitive
    +   *   Whether to use strstr() or stristr() which is case insensitive.
    +   */
    

    Better as:

    TRUE to use ... ; FALSE to use ....

  30. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   *   Whether to use strstr() or stristr() which is case insensitive.
    +   */
    +  public static function assertAllMatch($pattern, $traversable, $case_sensitive = FALSE) {
    +    if (static::assertTraversable($traversable)) {
    

    Missing @return.

  31. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Static::assertMatch() will be faster - use it when possible.
    

    Use @see to connect related methods. Also do not use Static::something because this is not real. Use the full namespaced class/method name.

  32. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   *   String[] where all members match expression needed to pass.
    

    See previous method comment on same param and this needs a @return too.

  33. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * You can overload this function. The first argument is required and is the
    

    See previous comment on "overload".

    Overload is actually something very different, not how you are using the term in this documentation.

    https://en.wikipedia.org/wiki/Function_overloading

  34. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * Note that if it is important enough to your code that a collection be
    +   * tested to be a collection of objects then it is important enough to give
    +   * all of those objects a common interface and test for just that interface.
    +   * This method is written to allow for other tests to minimize potential
    +   * disruption, but they should be used sparingly if at all.
    +   *
    

    So do you really need this????

  35. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,318 @@
    +   * @code
    

    Before @code a line describing what it is like "Here's an example:" is nice.

  36. +++ b/core/lib/Drupal/Component/Assertion/AssertionException.php
    @@ -0,0 +1,23 @@
    + * Contains Drupal\Component\Assertion\AssertionException.
    

    Namespace should start with \

  37. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * Contains Drupal\Component\Assertion\BaseAssertionTestingTrait.
    

    Namespace needs to start with \ here

  38. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * Methods for unit testing code which uses the PHP assert() function.
    

    I think we decided not to call it a function?

  39. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * Before we begin note the following - the term "assertion" is used throughout
    

    You can omit "Before we begin". Just write the docs.

  40. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * Now, by default both PHPUnit and Simpletest will stop when a runtime
    

    Omit "Now" too.

  41. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * \Drupal\Component\Assertion\Assertion you should write a unit test for that
    

    Could use a comma here after the Assertion class name.

  42. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * This trait establishes an assert() callback that logs assert() statement
    

    You said you would call it "runtime assertion" but now you're just calling it "assert()".

  43. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * implementing this trait check this log to see if the expected assert
    

    again

  44. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * dieOnRaise flag to TRUE to cause a
    

    Is the DieOnRaise flag part of some class? If so, add the class name to this with namespace.

  45. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * Drupal\Component\Assertion\AssertionException to be thrown.
    

    Start classes with full namespace starting with \ in docs. Always.

  46. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    + * to the implementing class.
    + *
    + * @code
    

    End line with : and omit blank line before the @code.

    Also technically traits are not "implemented". That is for interfaces.

  47. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +  protected $assertionsMustBeStrings = 'The first argument to assert()
    +    should always be a string even though PHP permits other argument types for
    +    two reasons: First, those arguments will increase overhead even when
    +    runtime assertions are turned off. Second, assertion-aware unit tests
    +    require a code string in order to label the runtime assertion.';
    +
    

    Why is this string multi-line? Normally we do not break up strings this way. Harder to scan.

    Also in the 3rd line, : should just be a .

  48. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Flag to throw an exception immediately on failure.
    +   *
    +   * @var bool
    +   */
    +  protected $dieOnRaise = FALSE;
    +
    

    Why is this called dieOnRaise if it throws and exception? That is not really "dying".

    This needs more explanation.

  49. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * @var String[]
    

    There is no such thing as String[] in PHP. Probably you mean string[].

    Maybe also explain what is in these strings?

  50. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Callback handler for assert() failures during testing.
    

    This is not following function docs standards (start with a verb etc.) Needs param/return docs too.

    Also this is a callback, not a callback handler.

  51. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Note - despite the name this is not an assert condition you call during
    +   * a unit test!
    +   */
    +  public function assertCallbackHandle($file, $line, $code, $message = '') {
    +
    

    Maybe you could think of a better function name in that case?

    And why does it include the word "handle" ? That seems very odd. It isn't a callback handler, it is just a callback.

  52. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +    // We print out this warning during test development, and since the
    +    // automated tests run in strict mode it will cause a test failure.
    +    if (!$code) {
    +      print ($this->assertionsMustBeStrings);
    +    }
    +
    

    huh?

  53. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Start assertion handling for the test.
    

    Start -> Starts

  54. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Call this from setUp()
    +   */
    +  protected function startAssertionHandling() {
    +    assert_options(ASSERT_WARNING, FALSE);
    +    assert_options(ASSERT_BAIL, FALSE);
    +    assert_options(ASSERT_CALLBACK, [$this, 'assertCallbackHandle']);
    +    $this->assertionsRaised = [];
    +    return FALSE;
    +  }
    

    Why does this even have a return value if it is always FALSE? And if this is left in the code, document it.

  55. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Suspend assertion handling.
    

    Suspend -> Suspends

  56. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Cease handling assertions and clear the way for the next test.
    

    Cease -> Stops

  57. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Call this from tearDown()
    

    Needs to end in .

  58. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Check if the runtime assertions specified where failed.
    

    um... not sure what this means, garbled? typos?

    Also Check -> Checks

  59. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * This function can be overloaded. Runtime assertions should be passed in
    

    overloaded is not the right term here. See above.

  60. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +  abstract protected function assertAssertionsRaised();
    

    Probably needs a @return too?

  61. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   * Insure no runtime assertions where thrown.
    

    Insure -> Asserts that
    where -> were (I think?)

    Unless this method is selling insurance. ;)

  62. +++ b/core/lib/Drupal/Component/Assertion/BaseAssertionTestingTrait.php
    @@ -0,0 +1,155 @@
    +   */
    +  abstract protected function assertAssertionNotRaised();
    +
    

    What does this return?

  63. +++ b/core/lib/Drupal/Component/Assertion/PHPUnitTestingTrait.php
    @@ -0,0 +1,33 @@
    + * Methods for testing the internal PHP assert function in PHPUnit.
    

    not calling it a function right?

  64. +++ b/core/modules/simpletest/src/AssertionTestingTrait.php
    @@ -0,0 +1,34 @@
    +trait AssertionTestingTrait {
    

    Looking at this one vs. the PHP Test one, I am not really sure why they need to be different?

  65. +++ b/core/modules/simpletest/src/AssertionTestingTrait.php
    @@ -0,0 +1,34 @@
    +  use BaseAssertionTestingTrait;
    +  /**
    +   * {@inheritdoc}
    

    Probably needs a blank line here?

  66. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,250 @@
    +use stdClass;
    

    really?

  67. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,250 @@
    +   * Test asserting all members are strings or objects with __toString().
    

    All these methods first verbs should be "Tests" not "Test". About half of them are fine; half not.

  68. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,250 @@
    +  /**
    +   * Call me sometime.
    +   *
    +   * This method exists as part of the is_callable test.
    +   */
    +  public function callMe() {
    +    return TRUE;
    

    Huh?

  69. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,250 @@
    +  /**
    +   * Call me static.
    +   *
    +   * Again, just a testing method.
    +   */
    +  public static function callMeStatic() {
    +    return TRUE;
    

    Huh? This isn't good documentation. Especially the first line.

  70. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * Drupal uses assert statements to perform validations that are only
    

    I thought we were calling them something other than "assert statements"? let's be consistent.

  71. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * Here we turn them back on which should be sufficient for most projects,
    

    needs comma after "on".

  72. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * will not be ran unless you modify the .htaccess file. We also set the
    

    grammar "will not be ran" is not right. Fix tenses.

  73. +++ b/sites/example.settings.local.php
    @@ -12,6 +12,20 @@
    + * will not be ran unless you modify the .htaccess file. We also set the
    + * system to halt on assert fail, just in case.
    + */
    

    just in case what?

jhodgdon’s picture

I propose that I redo #285 in the form of a patch/interdiff. ;)

jhodgdon’s picture

Regarding that, let me know if you are working on a new patch because it would be a pain to have to try to merge...

Aki Tendo’s picture

After sleeping on it a bit, and realizing there is still yet a bit of work on this anyway documentation-wise I'm going to bite the bullet and revisit the assertion trait part of this patch to cause PHP 5.x to behave in parallel to PHP 7.0's default behavior to allow unit tests involving assertions to be forward compatible.

https://wiki.php.net/rfc/expectations

jhodgdon’s picture

OK. When it comes to your next patch, I will again review the docs but this time I'll put my review in the form of a patch/interdiff. In the meantime, you might want to read through the previous review because there are docs standards mentioned there that you could follow in your next patch. Thanks!

Aki Tendo’s picture

I'm about halfway through that list and I do appreciate the help with this greatly though. One thing though, "Assert" is a verb. I'm using the wrong tense of it, but it is a verb.

Also, runtime assertions are going to be a fairly large group of functions so some conventions need to be established on their use as a group for clarity. Since these functions are used by the assert() statement they must always return boolean -- repeatedly documenting this for every assert statement will rapidly become repetitive. Also, at a minimum they take one argument - the variable that is being checked, and it must allow a mixed datatype. In the draft I wrote I only documented the exceptions to the general rule. I'll go ahead and doc them all though.

jhodgdon’s picture

Yes, Assert is a verb, sorry if I didn't look like I agreed with that. The only one I think I complained about that wasn't a verb was ... item 50 & 68/69. And regarding the "they're all pretty similar", keep in mind someone isn't looking necessarily at "all of them", but maybe just at one of them at a time. This is why our documentation standard is that all parameters and return values need documentation on everything.

Alternatively, you could put "See ____ for documentation" but it's really not any more work to copy and paste the standard param/return docs if they're really all the same than to copy/paste that line, is it?

Aki Tendo’s picture

Ok, PHP 7 mods made. Code base shrinking further. Let's see what all breaks.

1-4. Done.
5-8. This area rewritten.

8. Actually, no. This is a known bug, and as the new draft of the documentation points out has only now been fixed in PHP 7.

9. Done.

10. Done
11. Rewritten.
12-14. Done.

15. "Magic" has a special meaning in PHP - they are functions that cause the engine to behave a certain way. __toString() is called whenever the object is handled in a string context. It is pertinent to this assertion that the programmer be reminded that __toString() is one such function with this special handling.

16. Done.

17. Rewritten to clarify what is meant by "associative array". Also added a @see to the PHP manual page on arrays.

18. Params added.

19. A strict array's keys will always be 0 to X-1 where X is the count of elements in the array. As innocuous as this check sounds, it's the sort of check that would have blocked https://www.drupal.org/SA-CORE-2014-005. PHP's flexible array handling can be a double edged sword.

20. Line dropped.

21. See reference added.

23. I had asked how I might do this in IRC a few days ago. Good to know.

24. Rephrased.

25. See test failure of patch version at #276. I can't give the full path name because it cause the component test to fail - the test sees the core namespace class path and doesn't understand that the code is part of a passing comment and not part of executing code. I'll just drop the example.

26-30. Done

31. Dropped.

32. Done.

33. Loosely speaking, "overloading" a function applies anytime the argument signature is varadic either by datatype or number of arguments. That other languages require multiple copies of the method to be written to deal with this situation is immaterial to the term. I will avoid using it though since it very rarely comes up in PHP.

34. Rewritten for clarification, but it is needed since the risk of misusing the method is fairly high.

35. Done.

36-65. These classes have been deleted.

66. I prefer new stdClass(); to new \stdClass(), but the omission of the beginning \ requires a use statement. Same with ArrayObject.

67-69. Fixed.

71-75. Rewritten.

Aki Tendo’s picture

Status: Needs work » Needs review
dawehner’s picture

Just a quick question. Do those tests run, in case you execute them via phpunit directly? I don't see how they would be though, to be honest.

yched’s picture

Sorry for the nitpick :

assertAllMembersAre('is_int', $traversable) reads a bit weird grammatically - two verbs.
Since the more specific methods are assertAll[Property]($traversable), why not just assertAll($callback, $traversable) ?

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
28.89 KB
5.57 KB

Ok, Trying to have that AssertionException handler in three spots is asking for a maintenance headache, so refactored - though unfortunately the refactor forces a new include file to be made.

@dawehner - as long as the bootstrap file is included they'll run. Without the bootstrap they'll run - on PHP 7 - in theory.

@yched - good idea. Will incorporate into the next revision.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
28.92 KB

Added group to new test.

dawehner’s picture

@dawehner - as long as the bootstrap file is included they'll run. Without the bootstrap they'll run - on PHP 7 - in theory.

Ah yeah that is totally fine!

  1. +++ b/core/core.api.php
    @@ -983,6 +984,54 @@
    + * @defgroup php_assert PHP Runtime Assert Statements
    + * @{
    + * Use of the assert() statement in Drupal.
    

    Do you mind mentioning that by default we throw exceptions when an assertion is not met?

  2. +++ b/core/tests/bootstrap.php
    @@ -89,3 +89,7 @@ function drupal_phpunit_register_extension_dirs(Composer\Autoload\ClassLoader $l
    +// Set up runtime assertion handling and any other common test environment
    +// concerns.
    +require_once __DIR__ . '/../includes/testing.inc';
    

    I'm curious whether it would be better | worse to call that from within the base unit test class. Do you have an opinion about it?

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
29.06 KB
5.47 KB

Previous test fail caused by an issue that may need to be handled by a separate ticket - apparently the phpunit test runner under the simpletest module doesn't define DRUPAL_ROOT.

1. Documentation of this added.
2. The unit test of the Assertion Component itself extends directly from PHPUnit_Framework_TestCase in order to maintain independence from Drupal. I presumed the other components do the same which is why I put it in the bootstrap instead of the root test case.

dawehner’s picture

I presumed the other components do the same which is why I put it in the bootstrap instead of the root test case.

That is a totally good point. Thank you.

Aki Tendo’s picture

Doc changes only, no new code, so this should pass as well.

dawehner’s picture

I really like the current state of the patch. Here is just a list tiny nitpicks before an RTBC.

  1. +++ b/core/includes/testing.inc
    @@ -0,0 +1,39 @@
    +<?php
    +/**
    
    +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,412 @@
    +<?php
    +/**
    ...
    +   *   Variable to be examined.
    +   * @return bool
    
    +++ b/core/tests/Drupal/Tests/AssertionThrowTest.php
    @@ -0,0 +1,36 @@
    +class AssertionThrowTest extends UnitTestCase {
    +  /**
    
    +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,246 @@
    +<?php
    +/**
    

    Nitpick: new line missing

  2. +++ b/core/tests/Drupal/Tests/Component/Assertion/AssertionTest.php
    @@ -0,0 +1,246 @@
    +use stdClass;
    

    nitpick: We use "\$class_name" instead inline

Aki Tendo’s picture

Thanks.

1. I ran PHPCS over the thing one more time - it found more than new line issues. Did it catch these? Also, it wants there to be phpdoc information for the declaration of AssertionException in the testing file, but is this needed since AssertionException is a core class in PHP 7?

2. Done.

Aki Tendo’s picture

Previous patch submission has an unrelated patch mixed in by mistake. Do not review (test may likely fail anyway). Fixing this now. The interdiff is accurate though.

EDIT: The problem is I forgot to run git rebase. I'm going to read over the patch once again before submitting.

Aki Tendo’s picture

K, fixed. Found an outdated reference in the core api file - the example.settings.local.php file just links up to the testing.php file now, it has no additional information.

Aki Tendo’s picture

One last adjustment I'd like opinions on - the name of the assertion assisting class is still awkward to me -- What about calling it "Inspector" since that's all it does - inspect things.

So instead of this:

assert('\Drupal\Component\Assertion\Assertion::allAreStrings($array)');

We'd have

assert('\Drupal\Component\Assertion\Inspector::allAreStrings($array)');

I think the second reads better and is worth the headache of the change for me (I have to rewrite *all* child tickets to account for the name change). Thoughts?

Aki Tendo’s picture

Issue summary: View changes

Noted the PHP 5.x/7 behavior in the issue summary.

Aki Tendo’s picture

306 failed as expected due to lack of rebase. 308 is current patch

Crell’s picture

A few trivial nits. I don't think they're commit blockers but would be nice for polish's sake. For my part, I think we're done here. Thank you, Aki, for your tenacity!

  1. +++ b/core/core.api.php
    @@ -983,6 +984,58 @@
    diff --git a/core/includes/testing.inc b/core/includes/testing.inc
    

    This file seems like a good thing to spin off as a stand-alone BC shim via Composer. Not a blocker for this issue, just more encouragement to do so. :-)

  2. +++ b/core/lib/Drupal/Component/Assertion/Assertion.php
    @@ -0,0 +1,411 @@
    +  public static function assertAll($callable, $traversable) {
    

    Non-blocker: Since we're on PHP 5.5, callable is a safe type to hint against for $callable. Let's do that.

  3. +++ b/core/tests/Drupal/Tests/AssertionThrowTest.php
    @@ -0,0 +1,36 @@
    +    // We shouldn't reach this line.
    +    $this->assertTrue(FALSE);
    

    Technically unnecessary, as PHPUnit will fail for us if the expected exception doesn't get thrown.

Aki Tendo’s picture

The shim stuff now has it's own ticket. That leaves turning assertions on during development and off on production; and the inspecting class, in this patch.

No interdiff since the class name change would make for an interdiff the same size as the main file.

Crell’s picture

Um, I wasn't suggesting it move in this patch... Just that a general composer package would be useful for others. :-)

Why rename the class?

Aki Tendo’s picture

Well, moving the testing.inc file to a more specific php7shim.inc makes sense on its own I think. Separation of concern. As far as the class name change - that was discussed some in IRC but it comes down to a personal preference to getting code to read as naturally as possible, see #309.

Aki Tendo’s picture

Status: Needs work » Needs review
Aki Tendo’s picture

I feel stupid.

Aki Tendo’s picture

Status: Needs work » Needs review
dawehner’s picture

Um, I wasn't suggesting it move in this patch... Just that a general composer package would be useful for others. :-)

I totally agree, but this should not block us from moving forward here. We can always extract it later and then pull it in.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great to me now! :)

dawehner’s picture

@Aki Tendo
Thank you so much for pushing that issue and reinventing it multiple times.

alexpott’s picture

Here are some docs fixes that I think we should make - leaving at RTBC because it is only docs.

  1. +++ b/core/core.api.php
    @@ -983,6 +984,57 @@
    + * The Drupal project primarly uses runtime assertions to enforce the
    
    +++ b/sites/example.settings.local.php
    @@ -12,6 +12,24 @@
    + * The Drupal project primarly uses runtime assertions to enforce the
    

    primarly

  2. +++ b/core/core.api.php
    @@ -983,6 +984,57 @@
    + * complex data structures such as cache and render arrays. They are used to
    + * insure that methods return values in the documented datatypes. They also
    

    "They are used ensure that methods' return values are the documented datatypes." i think we need the possessive and to ensure not insure.

  3. +++ b/core/lib/Drupal/Component/Assertion/Inspector.php
    @@ -0,0 +1,411 @@
    +   * Asserts that all members are associative arrays.
    +   *
    +   * What PHP calls arrays are more formally called maps in most other
    +   * programming languages. A map is a datatype that associates values to keys.
    +   * Maps are sometimes called associative arrays, hence the name of this
    +   * method.
    +   *
    +   * @param mixed $traversable
    +   *   Variable to be examined.
    +   *
    +   * @return bool
    +   *   TRUE if $traversable can be traversed and all members are arrays.
    +   *
    +   * @see http://php.net/manual/language.types.array.php
    +   */
    +  public static function assertAllAssociativeArrays($traversable) {
    +    return static::assertAll('is_array', $traversable);
    +  }
    

    This method does not ensure that all members are associative arrays (as opposed to what is defined as a strict array below) - it's just testing that everything is an array. I think we should rename this to assertAllArrays.

  4. +++ b/core/lib/Drupal/Component/Assertion/Inspector.php
    @@ -0,0 +1,411 @@
    +   * specific about the data types of those keys and thefore it will be best to
    

    therefore

Fabianx’s picture

#325 makes sense to me, leaving at RTBC.

dawehner’s picture

+1

Aki Tendo’s picture

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

Regarding #3 above - the reason I had done that is technically a strict array is a form of associative array, just as all lions are felines, but I can understand not wanting to muddle things by being that technical. The comment blurb about maps and how PHP handles arrays is therefore moved to the documentation of assertAllStrictArrays as well as the see note about arrays in general.

The method name change requires a modification to the accompanying unit test that should be tested just to be sure. In addition to the changes above I corrected the 4 code examples in the Inspector class that where referring to it by it's previous name.

Thank you all for all of your support everyone. This has been a lot of fun and I've learned a lot from this - particular with git which prior to this project I could just barely muddle through.

Aki Tendo’s picture

I need coffee (regarding not labeling the interdiff as do not test). Also, I didn't notice Alex had also submitted a patch - sorry about that. Looking a the interdiff I missed one of the "primarily" corrections.

Aki Tendo’s picture

Status: Needs work » Needs review
FileSize
25.41 KB
8.11 KB

This patch consolidates #325 and #328 with one additional (hopefully final) correction on the documentation note for assertAllStrictArrays - it's not the data types of the keys we're concerned with - it's the datatype of the values belonging to those keys. The interdiff holds all changes subsequent to #320.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Component/Assertion/Inspector.php
@@ -104,36 +104,33 @@ public static function assertAllStringable($traversable) {
+   * Asserts that all members are arrays.
...
+  public static function assertAllArrays($traversable) {

Yeah that is much clearer

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cc34748 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

I think we should have a followup to enable assertions during our test runners (PHPUnit and Simpletest).

  • alexpott committed cc34748 on 8.0.x
    Issue #2408013 by Aki Tendo, alexpott, Fabianx, dawehner, Crell,...
Wim Leers’s picture

Aki Tendo, thank you so much for sticking with this issue and congratulations for getting it committed! I'm pretty sure many/most of your future Drupal contributions will seem trivial compared to this one!

Looking forward to finally being able to land #2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts) :)

twistor’s picture

Woot! This is going to be awesome.

almaudoh’s picture

Awesome!! Congratulations @Aki Tendo. This also happens to be one of the longest issues in recent times (and I just made it a comment longer :) ).

pfrenssen’s picture

Excited to see this in! Thanks @Aki Tendo for the great work!

Status: Fixed » Closed (fixed)

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

chx’s picture

Status: Closed (fixed) » Active
Issue tags: +Needs change record

SIGH. I am really grateful to @Aki Tendo for the great work for sure. But. This is basically a culture change, we never used assert() in core before and I don't think I've seen it in contrib either. This needs a change record for sure and a gdo/core announcement as well -- and if we have more ideas on how to spread the word then those too.

Aki Tendo’s picture

I'll draft up a full change record later today - I haven't written one before so I'll need someone to check over it.

Aki Tendo’s picture

Draft of change record prepared.

https://www.drupal.org/node/2569701

Crell’s picture

Holy cow that's a detailed change notice! Nice work, Aki!

I'd actually suggest moving most of that to the handbook, and then having just a short change notice that links to the handbook page. That way it's more easily findable in the future by new developers.

Aki Tendo’s picture

Then apply the changes please. It's easier for an editor to cut than add - and so I wrote as detailed a record as I could to make sure nothing was missed. I'm not sure what to include on this having not done one before.

Fabianx’s picture

I have gone ahead and slightly changed the title and published the change record.

We can always move it over to the handbook page, but chx is right that information should go out rather earlier than later.

  • alexpott committed cc34748 on 8.1.x
    Issue #2408013 by Aki Tendo, alexpott, Fabianx, dawehner, Crell,...
Wim Leers’s picture

Status: Active » Closed (fixed)
Issue tags: -Needs change record

https://www.drupal.org/node/2569701 was published back in September, almost 5 months ago. I think it's safe to say that the reasons for this being marked Active again in #340 have been addressed.