Runtime Assertion unit and functional testing

Ok, This should pass.

Group annotation missing.

Ok, We have a few paths we can go down here, each with upside and downside.

1. We go the route seen at comment # 308 in the assert tools patch, linking the shim code from settings.local.php, simpletest.module and the PHPUnit bootstrap.

https://www.drupal.org/node/2408013#comment-10135006

Pro: This is the smallest baby-step.
Con: Assertion failures that evaluate before the loading of the simpletest module or settings.local.php file will be errors in PHP 5 and exceptions in PHP 7. Stopping this requires an earlier include point.

2. We go the route seen in the patch already submitted here, patching into index.php file even before the autoloader runs.

Pro: This will ensure that Assertion behavior is consistent system-wide.
Con: Runs against the minimalist grain desired for the index.

---

Outside of the AssertionException issue it's desirable I think to convert E_RECOVERABLE_ERROR to TypeException. This either requires a set_error_handler call and then the existing Drupal error handlers have to be registered to not overwrite it like so

set_error_handler('_drupal_error_handle', E_ALL ^ E_RECOVERABLE_ERROR);

Or the conversion of E_RECOVERABLE can be done at the start of drupal_handle_Error.

There will be unit tests that need adjusting as well that are already trying to account for PHP 5 and 7's difference, but long term that should make those tests more stable.

As far as I know these are the only two error to exception conversions that are possible, all the other new PHP 7 exceptions will just flat out fail. I haven't finished reading the related RFC's though. If others exist the exception conversion must occur before the drupal error handler fires.

Input sought on this - I'm unsure of the best path here.

Ok, let's see what breaks (The Error handling test I expect, but I'm curious as to what else).

Hard to throw an exception that hasn't been defined.

There is a serious error in _drupal_error_handler_real() 's call to _drupal_log_error(). The second parameter it passes is the expression $error_level == E_RECOVERABLE_ERROR. This is the FALSE flag for drupal_log_error(), and it means this function has been treating all errors as non-fatal except the typeHint failures.

Since this patch is rerouting E_RECOVERABLE_ERROR to an exception throw it makes sense to go ahead and fix this line so that it throws the correct status to the log. Specifically:

$error_level & ~(E_NOTICE + E_USER_NOTICE + E_WARNING + E_USER_WARNING + E_DEPRECATED + E_USER_DEPRECATED + E_STRICT)

This is my intent - Create a bitmask of the nonfatals by adding their constants together, then does a bitwise check against the mask. So if $error_level is not in the set (i.e. is fatal) it returns true. Anyone want to verify that?

Agreed.

Attached is a grep of the PHP 5.6 codebase for E_RECOVERABLE. Addressing that whole list is something I'm not qualified to do alone so I'm going to scale back to just AssertError throws for the first version of the package.

Ok, here's the assertion error throw code as a composer require.

K. Got it now I think.

::kicks the testbot::

Fresh startover. It is working locally.

1. It should be able to run as long as phpunit is installed. It doesn't need a bootstrap. I could write a drupal companion to this function, but I don't think that's necessary - if this code where to start failing all unit tests running under PHP 5 with @expectedException AssertionError will start failing, so a regression can't sneak in under the radar.

2. I could make the assignment of the closure contingent on the status of assert_options, but this creates the problem of not having the callback in place if they get turned on later, like in settings.local.php. At the moment the whole file is unnecessary in production - but once the E_RECOVERABLE errors are added in then the catching of those throws in a uniform manner will become important.

On the other hand, if you're running PHP 7 this is a dead file, but opcode caching mitigates that. It would be nice if composer could be configured to conditionally load files and only load this when PHP version < 7.

In the end I think the registration of a closure that never gets called in production will have an insignificant impact on performance compared to the gain on not checking cache tags and context tags at all times - so we're giving back a very small piece of the performance gain on that front by doing it this way. I will agree that it's not ideal but all better solutions I can think of introduce setup complexity for site admins and developers I'd rather avoid.

Our other best option is to just go back to the pattern that was nearly RTBC'ed in the assert tools patch - use a require statement in bootstrap.php, simpletest.module and example.settings.local.php. Doing this would require the package to be modified in order to remove it's autoload information, but that autoloading was the point of putting it in the package in the first place.

Do we go that route or leave things as is? I really would like to get this out of the way so the cache optimizations can be brought up to speed.

Re-roll: e64a4c7 also modifies core/composer.lock so the previous patch will no longer work (it was tested just prior to e64a4c7). No operative changes from previous patch.

At dawehner's suggestion the package has been linked up to Travis CI and tested with it - which was a good thing since it wasn't running under PHP 7. This has been rectified and the package is now on version 1.0.2

This patch installs the shim, then tests it by going ahead and testing to make sure testing itself is possible in simple test. This also addresses the problems with the automated test runners.

I'm guessing I need to figure out what the fails in PHP 7 are from. That said, I've looked over the test results and the fails seem unattached to this patch. I'm considering running core itself directly through and see if its passing PHP 7 - last I had checked it was not.

K, new approach that doesn't use an outside package. This has the streamlining in two places since composer doesn't load includes/bootstrap.inc so PHPUnit's bootstrap needs the unifying code - but it will only ever be in these two places since if we discover another needed hook point the new function drupal_register_assert_handle() can be called.

No. The issue here is on how best to unit test runtime assertions and making sure they are running during functional tests regardless of the .htaccess setting.

None of the failures above on 7 are related to this patch at all, unless such notices are being thrown as AssertionErrors. I doubt this - it's undocumented behavior if it is what is happening.

Queing for retest to see how many of the unrelated PHP 7 funny business errors is still out there and start attacking them so this can run entirely clean.

@stefan

Can we call drupal_register_assert_handle() here instead?

No. That function is defined in includes/bootstrap.inc and PHP Unit's bootstrap does NOT load that file.

Why don't we just check on regular runtime if asserts are active and then set the assertion handler.

Since assertion status can be changed at runtime we have to insure that the handler is set when assert_active is set. We can do this for drupal code and I can modify the patch appropriately. If someone sets the assert_active flag after the handler's setting code runs then behavior will be the default for the platform, which isn't necessarily a bad thing but something to be aware of.

Before I do a new version though I'd like to know which route is warranted -

1. Use a package as seen in #24, having composer set up the handler as a global property
2. Use drupal call functions as seen in #28

Also a third option -- I know there was talk of allowing modules to have composer files. The most efficient way to do this is to attach the handler package to the simpletest library so that it is loaded only when that module is enabled. Incidently, this would also be a very sneaky way of making a BooBoo or Whoops module since they could use composer to load the files they need to have in place before the main script is underway - essentially composer becomes a form of hook_boot()

I'm not married to any approach but I'd like to know which one is favored before proceeding further.

EDIT: What's important isn't that the handler be registered at all times, just during the unit tests so that the test can be written in a uniform manner. That's a strike against #1. If #3 is possible I like that but #2 is the likely one.

Speaking with xjm in IRC we'll go with #2. I have some ideas on a smoother implementation he helped with.

Did this next patch as a clean rebuild so I can't easily do an interdiff. Here's a rundown of the changes with remarks regarding comments above.

core.api.php
Single change here is to bring the Runtime assertion docs in line with what PHP 7 actually does - they changed the name of the exception from AssertionException to AssertionError at some point.

b/core/lib/Drupal/Component/Assertion/Handle.php
New file. This will be the home of the registering code for the handler allowing it to be lazy loaded. The file does have the quirk of being double namespaced like some tests but this is unavoidable - AssertionError must reside at the root namespace.

@stefan_r -- At #42 you proposed overriding the constructor of Exception for AssertionException. That can't work because $this->file and $this->line are private, further if you do not invoke the parent::__constructor the object doesn't properly register with the runtime as an exception object and everything crashes. I strongly, strongly recommend staying away from modifying how the exception works - it should be enough that the assertions have the same class so that PHPUnit and simpletest can catch them either through annotation or the catch statement. So I did not incorporate this change into what I hope is the final patch.

I did incorporate simply passing the error code along, but I don't think that's going to work - I think PHP 7's default message string includes the line and file but I'm not sure of the exact wording - I am building a PHP 7 environment this weekend to figure it out and get this finalized.

@alexpott

Why don't we just check on regular runtime if asserts are active and then set the assertion handler. This means if someone sets up assertions in .htaccess or wherever it just works.

While I answered this in #45 I also misunderstood the question somewhat, so let me go over this again now that I'm thinking straight.

We can't both set this up at runtime and also lazy load the code. The clean way to run this code at the earliest opportunity is through the package approach seen in the patch at #24. I can modify that package to check if ASSERT_ACTIVE is set, but if it isn't and is then set true later in the code the callback won't be present and this could cause confusion. The ability to set the callback when ASSERT_ACTIVE changes is therefore needed and that in turn requires the code that sets the callback to live in a function (patch at comment #28) or with a static class (patch as of this comment). Neither of these options mesh well with a composer package so when in the runtime do we make the check? As early as possible, but we don't want to mess with index. DrupalKernel is a maze at the moment. In short there's no good place other than in a composer package to insert the assert handle registering code. That's ruled out by not wanting the code to be invoked during production so that leaves us with a function include, or a lazy loaded static class function. I believe the lazy load approach has the least overhead and is best.

It is with these problems in mind that I removed the ASSERT_ACTIVE check in the function so that if it gets called the handler is going to be registered. It does mean that if we turn assertions on at runtime we have to call for the handler. This occurs at three places - DrupalKernel for the webtest runners, PHPUnit's bootstrap, and example.settings.local.php for dev environments. The first and last of these set ASSERT_ACTIVE to TRUE, and PHPUnit bootstrap inherits the php.ini setting which is also TRUE by default. This exposes a corner case of changing the .htaccess setting and not having a handler but I don't believe this is an especially large problem.

The alternative given our current config approach is register the handler at all times making it a wasted action in production since the callback will never be called. This was the primary objection to the patch at #24. There is no current way to have composer conditionally load the file - even if there was how would we signal this? Such auto loads are processed way before any drupal specific work begins.

Let's step back a moment - why are assert statements going into the code in the first place? To make life easier on the module developers first, then the core developers. Setting ASSERT_ACTIVE to TRUE in settings.local.php is going to cover external module developers in full and the work on the internal modules as well. Further, I don't think there is an assert in the code base yet that could be failed before settings.local.php is loaded. Even if there was the default behavior of triggering an E_WARNING error should be sufficient. This patch is only really needed when a unit test needs to be written where the assert statement must fail to fulfill the test; and to make sure runtime assertions are on during the web tests. The former is already a very rare situation - unit testing assert statements is usually akin to unit testing unit tests: redundant. It does come up as can be seen in the presence of such tests in Drupal, but its a corner case to begin with. Finally, with the changes in the current patch PHPUnit will be able to conduct a test, but simpletest will not so that in my mind makes this a very to extremely rare possibility.

I believe the patch as it stands deals with the vast majority of our potential use cases, enough to warrant inclusion after another round of code review for spell checks (assuming the tests themselves come back clean). The few problems that do remain are strongly tied to problems with Drupal's error handling and environment setup system to begin with, and they will go away once those problems are addressed - but they are big problems that have already been pushed out to 8.1 minimum for good reason. What is here I think works for now and keeps things moving forward.

Thoughts?

Oops.

At this point I'm beginning to doubt 8.0.x is clean under PHP 7. Let's find out.

Ha! Head isn't working in PHP 7 yet. Same number of fails as the sanity check. So, I guess this means this is clear for final review.

Since this patch throws an AssertionError using an ASSERT_CALLBACK the value of ASSERT_WARNING is irrelevant in th e places where the handler is registered. PHP 7 introduces the ASSERT_EXCEPTION flag, and this patch sets that flag to 1 in PHP 7 and doesn't register a handler for it since one isn't needed for the desired effect.

Drush isn't affected by the .htaccess setting that was introduced in the assert tools patch - it will continue to function according to the php.ini default. My thought is that's ok, admin's savvy enough to be using drush should be comfortable making the necessary change to their php.ini file on production boxes to set assert.active to 0.

If drush uses it's own php.ini file then assert.active could be set there for a more uniform default - or the flag could be set according to whether drush is in test mode. But it is that setting, not assert_warning, that should be changed. assert.warning only deals with *how* the message gets delivered to the user.

In an idea world all users would understand that production machines should have assertions turned off, negating the need to have any assert setting declaration in the assert tools patch. That way drupal just goes along with the machine settings as is. But PHP ships with assertions on by default and most users don't change the setting since they don't understand what it is for (understandable). This creates the problem that if Drupal makes full use of assertions for runtime validating then, over time, they'll cause a noticable performance hit. People aren't going to blame their bad setting on this, they're going to blame drupal, fair or no.

Yes, If drush loads settings.local.php that assert_option call goes into effect from that point in the code onward. Turning warnings off when registering an exception thrower seems a reasonable step and necessary to get the tests to behave I'll guess.

Ok, code from 42 ported in and corrected (I was wrong the crashes where off the last two parameters being non-optional but following parameters that are optional - the fix is to make all of them optional). As for #53

1. Drupal currently has no code on the root namespace - so autoloading this from its own file would require a reconfig on composer. That's way out of scope here.

2-5 fixed

6. The consensus seems to be we don't want this handler registered if it won't be used for performance reasons.

@damian This patch now turns assert.warning off when Handle::register is called.

Looks great to me. Can someone who hasn't touched the code give this a final RBTC please?

Normally file and line get set to the point of the throw by the engine. Wanting to change these parameters is a bit of an oddball situation - I can see why they didn't expect it and therefore didn't make it an argument of the default constructor.

Updating Issue summary to document final course of action at a glance without needing to read through the whole thread to figure out what happened to the patch, in case we need to revisit this at some later date.