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
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. |
Comment | File | Size | Author |
---|---|---|---|
#331 | interdiff-331--do-not-test.diff | 8.11 KB | Aki Tendo |
#331 | 2408013-331.diff | 25.41 KB | Aki Tendo |
#328 | interdiff-327.diff | 5.93 KB | Aki Tendo |
| |||
#328 | 2408013-327.diff | 25.42 KB | Aki Tendo |
#325 | 2408013.325.patch | 25.25 KB | alexpott |
Comments
Comment #1
Wim LeersComment #2
Aki Tendo CreditAttribution: Aki Tendo commentedNote 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.
Comment #3
Aki Tendo CreditAttribution: Aki Tendo commentedWow. 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
Comment #4
Aki Tendo CreditAttribution: Aki Tendo commentedAttaching 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
Feedback welcome.
Comment #5
Aki Tendo CreditAttribution: Aki Tendo commentedComment #7
Aki Tendo CreditAttribution: Aki Tendo commentedOk, second try at making a patch, bear with me.
Comment #8
Aki Tendo CreditAttribution: Aki Tendo commentedIncidentally, the assert statement for the moment is
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.
Comment #9
neclimdulIf 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.
Comment #10
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedA question, and perhaps this is academic, but what is the benefit of using assert over using Exceptions to do exception handling?
Comment #12
JeroenTIs this issue related? #2376153: Use whoops for exception handling
Comment #13
Aki Tendo CreditAttribution: Aki Tendo commented@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 -
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
Comment #14
Aki Tendo CreditAttribution: Aki Tendo commentedTo further underscore the above let's look at one of the assertions I placed in the patch in the TwigExtension class
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.
Comment #15
Aki Tendo CreditAttribution: Aki Tendo commentedChanging to a cleaner title.
Comment #16
Aki Tendo CreditAttribution: Aki Tendo commentedComment #17
Aki Tendo CreditAttribution: Aki Tendo commentedI'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.
Comment #18
Aki Tendo CreditAttribution: Aki Tendo commentedComment #19
tim.plunkettThis 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.
Comment #20
Aki Tendo CreditAttribution: Aki Tendo commentedI'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.
Comment #21
mpdonadioOut 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.
Comment #22
Aki Tendo CreditAttribution: Aki Tendo commentedAt 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...
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:
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
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?
Comment #23
Aki Tendo CreditAttribution: Aki Tendo commentedNot quite ready to release the next patch, but I have a screen capture of the thing in action.
http://screencast.com/t/ih41Wi6YCd
Comment #24
pfrenssenA half patch is better than no patch ;)
Comment #25
Aki Tendo CreditAttribution: Aki Tendo commentedOk, 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.
Comment #26
Aki Tendo CreditAttribution: Aki Tendo commentedComment #28
mpdonadioDoesn't Dries need to approve all new libraries, or is there an exception for additional Symfony components?
Comment #29
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #30
Aki Tendo CreditAttribution: Aki Tendo commentedComment #31
Aki Tendo CreditAttribution: Aki Tendo commentedQuestion - 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
I have added immediately proceeding the return statement..
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.
Comment #32
joelpittet@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
Comment #33
joelpittet@Aki Tendo the Issue summary is great by the way, this sounds like a very interesting idea! Color me interested:)
Comment #34
pfrenssenThanks for reworking the patch without depending on additional libraries, this will make it more likely to be accepted. This looks very interesting.
Comment #35
Aki Tendo CreditAttribution: Aki Tendo commentedThis 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.
Comment #36
Aki Tendo CreditAttribution: Aki Tendo commentedHiding older patches.
Comment #37
pfrenssenI 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?Comment #38
Aki Tendo CreditAttribution: Aki Tendo commentedThat 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.
Comment #39
Aki Tendo CreditAttribution: Aki Tendo commentedComment #40
Aki Tendo CreditAttribution: Aki Tendo commentedOver 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.
Comment #41
Aki Tendo CreditAttribution: Aki Tendo commentedTitle change, typos.
Comment #42
Aki Tendo CreditAttribution: Aki Tendo commentedAfter 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.
Comment #43
Aki Tendo CreditAttribution: Aki Tendo commentedApplying proper formatting - https://www.drupal.org/issue-summaries
Comment #44
Aki Tendo CreditAttribution: Aki Tendo commentedThis patch, should it pass automated testing, should be ready for inclusion.
Comment #45
benjy CreditAttribution: benjy commentedExtra white spaces around $message. Actually, this seems to be throughout the patch.
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
Comment #46
cilefen CreditAttribution: cilefen commented@Aki Tendo There is a Drupal standards checker for the command line. See https://www.drupal.org/node/1419988
Comment #47
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #48
larowlanShould be /**
would http_kernel be a better variable name?
needs a new line between { and /, and should be /**
should be a blank line before here.
Should have a full stop
Our standards is
our standard is
Our standard is for a one-line <80 char short summary. Subsequent lines go after summary. e.g.
no need for spaces inside parenthesis
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.
Comment #49
cilefen CreditAttribution: cilefen commentedI 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.
Comment #50
cilefen CreditAttribution: cilefen commentedThe 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.
Comment #51
pfrenssenI 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.
Comment #52
cilefen CreditAttribution: cilefen commentedTagging 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.
Comment #53
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #54
Aki Tendo CreditAttribution: Aki Tendo commentedComment #55
kerby70 CreditAttribution: kerby70 at Blink Reaction (now part of FFW) commentedI 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.
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.
Comment #56
Aki Tendo CreditAttribution: Aki Tendo commentedCode 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.
Comment #57
Aki Tendo CreditAttribution: Aki Tendo commentedA 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.
Comment #58
Aki Tendo CreditAttribution: Aki Tendo commentedComment #59
Aki Tendo CreditAttribution: Aki Tendo commentedWith 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.
Comment #60
Aki Tendo CreditAttribution: Aki Tendo commentedBugfix 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.
Comment #61
Aki Tendo CreditAttribution: Aki Tendo commentedOptimizations 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).
Comment #64
Aki Tendo CreditAttribution: Aki Tendo commentedNote to self - rerun all tests before making a patch :(
Comment #65
Aki Tendo CreditAttribution: Aki Tendo commentedComment #67
Aki Tendo CreditAttribution: Aki Tendo commentedRolling back to last known good.
Comment #68
Aki Tendo CreditAttribution: Aki Tendo commentedWith the one should be safe optimization.
Comment #69
Aki Tendo CreditAttribution: Aki Tendo commentedComment #70
dawehnerLet's first post a easier reviewable patch.
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.
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?
Can this documentation point to the 3 ways how it could happen? Just mentioning them without explaining what they are, is kinda sad.
We prefer to use static:: over self:: in order to allow subclasses to change the behaviour.
Ensure to make empty lines after the last use statement, and after the curly brace after the class line
Comment #71
Aki Tendo CreditAttribution: Aki Tendo commentedI misunderstood how Sourcetree builds patches. It isn't straightforward. This one will be cleaner.
Comment #72
Aki Tendo CreditAttribution: Aki Tendo commented1.
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.
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.
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.
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.
Comment #73
Aki Tendo CreditAttribution: Aki Tendo commenteddawehner '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
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.
Comment #74
xjmThanks @Aki Tendo! Great work here.
The beta evaluation in the summary is not correct. It says:
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.
Comment #75
Aki Tendo CreditAttribution: Aki Tendo commentedOk, 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.
Comment #76
Fabianx CreditAttribution: Fabianx commented+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.
Comment #77
Fabianx CreditAttribution: Fabianx commentedThat is a nit, but needs to be
assertion.html.twig
according to our naming standards.
Comment #78
Wim LeersAki 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!
Comment #79
alexpottI 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…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:
[Edit: improved comment around other costs on doing the patch.]
Comment #80
alexpottComment #81
Wim LeersSee #2454643: Optimize cacheability bubbling (Cache::mergeTags(), ::mergeContexts(), BubbleableMetadata).
Comment #82
catchI 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).
Comment #83
mpdonadio@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.
Comment #84
pfrenssenUpdating status, Alex has given feedback in #79.
Comment #85
xjmI'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.
Comment #86
Aki Tendo CreditAttribution: Aki Tendo commentedWell 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?
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.
Comment #87
Fabianx CreditAttribution: Fabianx commented#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 :).
Comment #88
star-szrI'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!
Comment #89
Aki Tendo CreditAttribution: Aki Tendo commentedThey 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.
Comment #90
Aki Tendo CreditAttribution: Aki Tendo commentedI 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?
Comment #91
dawehnerI 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.
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.
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.
Can we pass along the message coming from $e? Otherwise we potentially loose information, which would be helpful.
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.
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.
Comment #92
Aki Tendo CreditAttribution: Aki Tendo commentedIt'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.
Already done. I was having parse errors in the twig template last night and realized having that info is sorta important.
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:
As a result, the whole module message / module gathering carnival is avoided.
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.
Comment #93
Aki Tendo CreditAttribution: Aki Tendo commentedMinor updates to the issue summary.
Comment #94
cosmicdreams CreditAttribution: cosmicdreams commentedIs 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.
Comment #95
Fabianx CreditAttribution: Fabianx commented#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?
Comment #96
Aki Tendo CreditAttribution: Aki Tendo commentedI'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.
Comment #97
alexpottre #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.
Comment #98
Aki Tendo CreditAttribution: Aki Tendo commentedOk, here we go.
Comment #100
Aki Tendo CreditAttribution: Aki Tendo commentedDidn't know the comment docs could cause fails.
Comment #101
dawehner... As promised on IRC
Comment #102
Crell CreditAttribution: Crell commentedI 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? :-)
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. :-)
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.
Now *that* is a classy idea, all on its own! I like!
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.
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.
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. :-)
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.
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.
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.)
Comment #103
Aki Tendo CreditAttribution: Aki Tendo commentedTackling the above post points one at a time
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
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.
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.
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.
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.
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.
I'm not familiar with the JS side of the equation, I just mimicked verbatim what _drupal_log_error currently does.
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.
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.
Comment #104
Crell CreditAttribution: Crell at Palantir.net commentedAgreed. 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:
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.)
Comment #105
mpdonadioIt 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.
Comment #106
Crell CreditAttribution: Crell at Palantir.net commentedAh, thanks mpdonadio. I believe my previous statement regarding it being crazy-edge-case-only cases is still valid, then.
Comment #107
Aki Tendo CreditAttribution: Aki Tendo commentedYou'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...
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.
Comment #108
Aki Tendo CreditAttribution: Aki Tendo commentedComment #109
Crell CreditAttribution: Crell at Palantir.net commentedOK, 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?
Comment #110
Aki Tendo CreditAttribution: Aki Tendo commentedThere, 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.
Comment #111
almaudoh CreditAttribution: almaudoh commented+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
Comment #112
Fabianx CreditAttribution: Fabianx commentedIn 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:
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.
Comment #113
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #114
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #116
Aki Tendo CreditAttribution: Aki Tendo commentedGuess I need to silence logging during auto-testing.
Comment #118
Fabianx CreditAttribution: Fabianx commentedCan 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.
Comment #119
Aki Tendo CreditAttribution: Aki Tendo commentedSwitched out sad droop to svg, removed the tests being disrupted by having slightly different php versions, and providing a combined patch.
Comment #121
Aki Tendo CreditAttribution: Aki Tendo commentedTrying the cache patch again.
Comment #123
Aki Tendo CreditAttribution: Aki Tendo commentedOk, 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.
Comment #125
Aki Tendo CreditAttribution: Aki Tendo commentedThe 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.
Comment #127
Fabianx CreditAttribution: Fabianx commentedAki, 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.
Comment #128
Aki Tendo CreditAttribution: Aki Tendo commentedThe patch I thought would work last night failed because for whatever reason the AssertionException wasn't included in the patch.
Comment #129
Aki Tendo CreditAttribution: Aki Tendo commentedComment #130
Aki Tendo CreditAttribution: Aki Tendo commentedFaults patch updated with the changes to the assertion trait,
Cache tests further optimized, compare with and without assertions on.
Comment #133
Aki Tendo CreditAttribution: Aki Tendo commentedRenderer 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.
Comment #134
Fabianx CreditAttribution: Fabianx commented#133 If you feel those patches are ready, I can take on the profiling. Are they ready?
Comment #135
Aki Tendo CreditAttribution: Aki Tendo commentedYes.
Comment #136
Aki Tendo CreditAttribution: Aki Tendo commentedComment #137
Fabianx CreditAttribution: Fabianx commentedUnfortunately 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.
Comment #138
willzyx CreditAttribution: willzyx commented@fabianx you can use patch #15 from #2422101: CommentItem should override the generateSampleValue method and provide sample values for fix devel_generate
Comment #139
Fabianx CreditAttribution: Fabianx commented#138: Thank you very much! I will try that!
Comment #140
aspilicious CreditAttribution: aspilicious commentedCan 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
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.
Comment #141
aspilicious CreditAttribution: aspilicious commentedReading 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
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.
Comment #142
Aki Tendo CreditAttribution: Aki Tendo commentedScreenshot 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.
Comment #143
Aki Tendo CreditAttribution: Aki Tendo commentedThat 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.
Comment #144
aspilicious CreditAttribution: aspilicious commentedWell now you're really underestimating the process for frontend changes/additions.
Well than we have a bug ==> '/core/lib/Drupal/Component/Fault/drupal_error.png'
Thats not how it works. *Everything* should follow the standards, else it is not a standard.
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
Comment #145
mgifford@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.
Comment #146
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #147
Fabianx CreditAttribution: Fabianx commentedAnd here are the results for 50 nodes on the frontpage:
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.
Comment #148
Crell CreditAttribution: Crell at Palantir.net commentedAki: 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.)
Comment #149
Aki Tendo CreditAttribution: Aki Tendo commentedYou'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.
While it does, turning assertions off while working on something that they are bogging down - like a large migration - is elementary.
That's already accounted for - The patch makes assertions off the default setting in the .htaccess file.
Comment #150
dawehnerIf I undertand the issue correctly we would not turn them on by default but rather opt it in?
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.
That is a good point.
Just curious, is there a reason why we don't call out to the static method?
Comment #151
Wim LeersExactly.
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.
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.
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?
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.
Can we just remove this image? It's a nice touch, but likely controversial. Let's keep it simple.
Let's restore the original whitespace.
::mergeContexts()
, and just like it was originally.::validateTags()
:)Space again.
s/Defines Drupal\…/Contains \Drupal\…/
Always "Contains \Drupal\…".
Comment #152
dawehnerI 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.
Comment #153
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
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.
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.
Comment #154
dawehnerYeah 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.
Comment #155
catchThe 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).
Comment #158
Aki Tendo CreditAttribution: Aki Tendo commentedChanges from Wim's code review completed, now testing assertions on. Notes
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
Ok, removed.
Done. Let's see what breaks.
Other whitespace issues also addressed.
Comment #160
Aki Tendo CreditAttribution: Aki Tendo commentedComment #162
Aki Tendo CreditAttribution: Aki Tendo commentedLet'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.
Comment #163
Bojhan CreditAttribution: Bojhan commentedIeuww, what is this page? I dont really know what to review.
Comment #164
Aki Tendo CreditAttribution: Aki Tendo commentedComment #165
aspilicious CreditAttribution: aspilicious commented@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
Comment #166
Aki Tendo CreditAttribution: Aki Tendo commentedOk, 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:
Comment #167
Wim Leers#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:
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.
I thought 158 removed this?
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.
Shouldn't we just remove this method instead?
Can you expand these docs to describe such occasions? It's not obvious to me.
Also: s/occassion/occasion/ :)
Comment #168
Aki Tendo CreditAttribution: Aki Tendo commented1. 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
Comment #169
Aki Tendo CreditAttribution: Aki Tendo commentedAdded 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.
Comment #170
aspilicious CreditAttribution: aspilicious commentedThis 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.
Comment #171
Aki Tendo CreditAttribution: Aki Tendo commentedThey 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.
Comment #172
Crell CreditAttribution: Crell at Palantir.net commentedTo 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.
Comment #173
Aki Tendo CreditAttribution: Aki Tendo commentedDev 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.
Comment #174
mpdonadioI 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.
Comment #175
Crell CreditAttribution: Crell at Palantir.net commentedYes. 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.
Comment #176
Aki Tendo CreditAttribution: Aki Tendo commentedOptimization 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.
Comment #177
Fabianx CreditAttribution: Fabianx commentedYes, 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.)
Comment #178
Aki Tendo CreditAttribution: Aki Tendo commentedThe 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.
Comment #179
Aki Tendo CreditAttribution: Aki Tendo commented#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.
Comment #180
Aki Tendo CreditAttribution: Aki Tendo commentedLast night's changes to head necessitate a re-roll.
Comment #181
Aki Tendo CreditAttribution: Aki Tendo commented110 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.
Comment #183
Bojhan CreditAttribution: Bojhan commentedI am just wondering, can we use Seven CSS? This is independent from system failures, I assume.
Comment #184
Aki Tendo CreditAttribution: Aki Tendo commentedI'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.
Comment #185
Aki Tendo CreditAttribution: Aki Tendo commentedK, 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.
Comment #186
Aki Tendo CreditAttribution: Aki Tendo commentedComment #187
Aki Tendo CreditAttribution: Aki Tendo commentedRan some initial profiling of my own - I hope I got this right.
8.0.x as of this morning.
Patch applied, assertions on.
Patch, Assert Off
Comparison of the two most critical numbers...
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.
Comment #188
alexpott@Bojhan #183 - the whole point is to not use theme system - the less dependencies the assertion system has the better.
Comment #189
Bojhan CreditAttribution: Bojhan commented@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.
Comment #190
Aki Tendo CreditAttribution: Aki Tendo commented@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.
Comment #191
joelpittet@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
Comment #192
Aki Tendo CreditAttribution: Aki Tendo commentedI see no reason not to.
Comment #193
Aki Tendo CreditAttribution: Aki Tendo commentedUpdate title and summary.
Comment #194
Aki Tendo CreditAttribution: Aki Tendo commentedChanges for code standards.
Comment #196
Aki Tendo CreditAttribution: Aki Tendo commentedInterdiff still relevant - only index.php changed between this patch and 194
Comment #197
Aki Tendo CreditAttribution: Aki Tendo commentedComment #198
Bojhan CreditAttribution: Bojhan commentedI 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).
Comment #199
Aki Tendo CreditAttribution: Aki Tendo commentedSounds good to me. Where's the maintenance page in the system?
Comment #200
Wim LeersWe should confirm that on another machine, but if that is correct, that's awesome!
Comment #202
Aki Tendo CreditAttribution: Aki Tendo commentedMissed a use statement in the index, causing every simpletest using the web system to fail.
Comment #203
Aki Tendo CreditAttribution: Aki Tendo commentedAny progress on the profiling?
Comment #204
Bojhan CreditAttribution: Bojhan commented@Aki Teno IF you put the site in maintaince mode you should see it.
Comment #205
Aki Tendo CreditAttribution: Aki Tendo commentedK. 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.
Comment #207
Aki Tendo CreditAttribution: Aki Tendo commentedTrying again.
Comment #208
dawehner2% 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.
Nitpick: Our code style adds a new line here.
Should be {@inheritdoc}
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.
Another nitpick: We should have documentation here for those parameters.
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.
What is the resaon for this count() statement?
Can we somehow reuse the same code?
@inheritdoc on protected methods feels wrong.
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.
Comment #209
Aki Tendo CreditAttribution: Aki Tendo commentedI 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 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.
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.
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.
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.
All assert functions in both PHPUnit and Simpletest are protected. Both of these functions implement an abstract method in BaseAssertionTestingTrait
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
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.
Comment #211
Aki Tendo CreditAttribution: Aki Tendo commentedCacheContextManager mocks must have a assertValidTokens mock method that returns true, otherwise the assertion always fails. Tests adjusted.
Comment #212
Aki Tendo CreditAttribution: Aki Tendo commentedOk, green again. Still need profiling done.
Comment #213
Fabianx CreditAttribution: Fabianx as a volunteer commentedProfiling this now.
Comment #214
Aki Tendo CreditAttribution: Aki Tendo commented(posted to wrong issue ticket)
Comment #215
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedQuick re-roll
Comment #216
Aki Tendo CreditAttribution: Aki Tendo commentedIf it fails the most likely reason is a new test that has a cache_contexts_manager mock. This line is needed on those mocks.
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.
Comment #217
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented50 nodes, front page, render cache = off, page cache = off
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.
Comment #218
Aki Tendo CreditAttribution: Aki Tendo commentedExcellent. 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?
Comment #219
Crell CreditAttribution: Crell at Palantir.net commentedDuring 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.
Comment #220
Aki Tendo CreditAttribution: Aki Tendo commentedComment #221
Aki Tendo CreditAttribution: Aki Tendo commentedI 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.
Comment #222
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI very much like #221 as a proposal, that makes it independent from any d.org deployment and also gives most flexibility.
Comment #223
Aki Tendo CreditAttribution: Aki Tendo commentedThe 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
Comment #224
Aki Tendo CreditAttribution: Aki Tendo commentedAs Crell recommends I have decided to halt all further development on this until I have a green light to proceed.
Comment #225
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAssigning to alexpott for official framework manager review.
Comment #226
hass CreditAttribution: hass commentedI 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?
Comment #227
Crell CreditAttribution: Crell at Palantir.net commentedhass: 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.)
Comment #228
Aki Tendo CreditAttribution: Aki Tendo commentedNo, 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.
Comment #229
Crell CreditAttribution: Crell at Palantir.net commentedAki: 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. :-)
Comment #230
Aki Tendo CreditAttribution: Aki Tendo commentedAh. Ok (got crossed up there).
Still waiting for orders on how to proceed.
Comment #231
catchI 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).
Comment #232
alexpottLike @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:
... 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
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.
Comment #233
Aki Tendo CreditAttribution: Aki Tendo commentedYeah, 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.
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?
Comment #234
alexpottThanks @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:
Comment #235
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedUpdating the issue summary to bring it into line with the goals outlined in 233.
Documented or not, assert statements are in there. This is the return of a find in files command in sublime text.
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.
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.
Comment #236
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedNoticed some typos in the issue summary.
Comment #237
twistor CreditAttribution: twistor commented@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.
Comment #238
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI 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.
Comment #239
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOk, 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.
Comment #241
JeroenT@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.
Comment #242
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedYes. 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.
Comment #244
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedWrong namespace in test.
Comment #245
JeroenT@Aki Tendo,
Keep up the good work then! ;-)
Comment #246
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commented^ 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.
Comment #247
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedTypo corrections in comment text, no code changes.
Comment #248
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedDuring cache hardening and optimization I found a bad use statement in the simple test class.
Comment #249
Crell CreditAttribution: Crell at Palantir.net commentedIf I'm reading the patch correctly, this adds a redundant session.auto_start line.
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"?
I like the rest of this comment, though.
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.)
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.
PHPUnit is one word.
Comment #250
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOops. Fixed.
Done.
Thanks.
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.
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
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.
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.
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.
Comment #251
alexpottNeed 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.
"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."
@see sites/example.settings.local.php
The "now" is redundant.
Mis-spelt idiosyncrasies and unnecessary capital A on assertion.
Missing fullstop
Missing @return documentation.
If the lack of a break here is intentional can it be documented.
This should be a one line that says what it does.
"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.
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?
Extra spaces.
This appears never to be set to True - is it obsolete?
Not used
Missing a leading slash
"Drupal uses assert statements..."
Double space after . and lines are longer than 80 chars.
Comment #252
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commented1. 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.
Comment #253
alexpott10. 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.
Comment #254
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOk, changes for the above applied as well. Thanks.
Comment #255
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedLooked 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.
Comment #256
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedAt Alex's request, api documentation added.
Comment #257
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedRevised and condensed entry. Interdiff only, no new code.
Comment #258
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedEven further condensed at jhodgdon's request in IRC.
Comment #260
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedLast patch was an interdiff and should not have been tested.
Comment #261
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThis 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
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:
Postcondition assertions
These are assertions concerning the return of a function.
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.
Comment #262
jhodgdonUm, where's the new patch to review?
Comment #263
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedJust to avoid confusion, a re-roll.
Comment #264
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #265
jhodgdonWhew!
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!
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".
Normally we want documentation to preceed what it is documenting, so maybe this block should be moved up?
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?
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?
Would also want to disambiguate this topic title.
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".
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?
assert()
Also end this line in a : before a list.
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.?
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.
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...
tainted => unsanitized
Normally in these topics, we write out something like:
See https://whatever for more information on ...
assert()
assert()
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.
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."
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?
This needs punctuation, like "Property to test for; one of:"
Shouldn't this be two separate items?
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?
@return needs documentation line.
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?
This option is not documented in the @param
implement is misspelled
which -> that
Such should not be capitalized
This option is not documented in the @param.
Really, you want to allow this much flexibility?
again, this much flexibility? Why not just document what to pass in to get this test?
This option is not documented in the @param
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.
TRUE not true
This is a comma splice
Just a note that 'object' and 'array' are not technically "primitive" types, as suggested by the @param docs.
ummmm... not sure what this means?
Assertion-aware needs hyphen
assert()
We don't normally use * for emphasis in docs
maybe this should say something like "will cause major problems if left to continue" ?
Also "simpletest" is ... ???
needs comma after "errors".
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.
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.
Again... an "assertion"?
Traits don't have children. And you probably don't need to document that the trait is used in two or more different classes.
This sentence has some verb agreement problems.
What's an assert raise?
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.
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.
Odd name for this trait, if it is about assertion handling. Shouldn't it have the word Assertion in the trait name?
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
Collections? Wouldn't it just be one collection?
Should maybe say:
The captured assertions.
Normally we don't like @var array and prefer it to be @var \Some\Thing\Collected[]
Extra blank line here
Comment #266
jhodgdonOne 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
Comment #267
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commented1-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
Comment #269
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI hate renaming things. Something always breaks when renaming.
Comment #270
Crell CreditAttribution: Crell as a volunteer commentedAt 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.
Comment #271
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThat'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.
Comment #272
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedRewrote the Assertion Tools, breaking up the AllMembersAre function into discrete chunks.
Comment #274
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedWhy that static error didn't pitch locally I dunno.
Comment #275
Crell CreditAttribution: Crell as a volunteer commentedA couple of minor mostly non-blocking nits. I fully expect the next patch posted to be RTBCable. Nice work, Aki!
This docblock could benefit from an example. It's tricky to understand what you'd do with this method without reading the code.
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.
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.)
Good description! But a code sample would help here anyway. (Any time you have a pseudo-variadic function, a code sample helps.)
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.)
Comment #276
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThanks 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.
Comment #277
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#276: The last interdiff seems incomplete, maybe the comparison target was the last commit instead of the last patch you provided?
Thanks!
Comment #278
Crell CreditAttribution: Crell at Palantir.net commentedI 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.
Comment #279
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commented@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?
And also, I'd like to keep my tongue in cheek unit test joke.
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?
Comment #281
Crell CreditAttribution: Crell at Palantir.net commentedAki: 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.
Comment #282
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedMentioning 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.
Comment #283
alexpottI 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.
Comment #284
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThanks Alex.
Crell's refactor suggestion incorporated. Hopefully this is final draft.
Comment #285
jhodgdonThis 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.
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"?
Maybe a comma at the end of the first line would help?
has adopted => uses ?
The Drupal project writing standards say we should use serial commas. So needs a comma after "modules" on this line.
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"? ???
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?
comma after development.
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.
How about:
Use caution when using them with data that may be unsanitized.
for the last sentence here?
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.
inspect -> inspects
(see previous comment)
But actually.. this doesn't tell what the function does. Please document it.
This is not acceptable documentation for parameters -- needs descriptions.
See https://www.drupal.org/node/1354#param
@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.
Assert -> Asserts that
This also needs @param and @return docs.
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.
Assert -> Asserts that
Also needs @param and @return docs.
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?
Assert -> Asserts that
This also needs @param and @return docs.
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?
false -> FALSE
"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.
Assert -> Asserts that
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.
huh?
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.
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.
member -> members
which -> that
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.
Better as:
TRUE to use ... ; FALSE to use ....
Missing @return.
Use @see to connect related methods. Also do not use Static::something because this is not real. Use the full namespaced class/method name.
See previous method comment on same param and this needs a @return too.
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
So do you really need this????
Before @code a line describing what it is like "Here's an example:" is nice.
Namespace should start with \
Namespace needs to start with \ here
I think we decided not to call it a function?
You can omit "Before we begin". Just write the docs.
Omit "Now" too.
Could use a comma here after the Assertion class name.
You said you would call it "runtime assertion" but now you're just calling it "assert()".
again
Is the DieOnRaise flag part of some class? If so, add the class name to this with namespace.
Start classes with full namespace starting with \ in docs. Always.
End line with : and omit blank line before the @code.
Also technically traits are not "implemented". That is for interfaces.
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 .
Why is this called dieOnRaise if it throws and exception? That is not really "dying".
This needs more explanation.
There is no such thing as String[] in PHP. Probably you mean string[].
Maybe also explain what is in these strings?
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.
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.
huh?
Start -> Starts
Why does this even have a return value if it is always FALSE? And if this is left in the code, document it.
Suspend -> Suspends
Cease -> Stops
Needs to end in .
um... not sure what this means, garbled? typos?
Also Check -> Checks
overloaded is not the right term here. See above.
Probably needs a @return too?
Insure -> Asserts that
where -> were (I think?)
Unless this method is selling insurance. ;)
What does this return?
not calling it a function right?
Looking at this one vs. the PHP Test one, I am not really sure why they need to be different?
Probably needs a blank line here?
really?
All these methods first verbs should be "Tests" not "Test". About half of them are fine; half not.
Huh?
Huh? This isn't good documentation. Especially the first line.
I thought we were calling them something other than "assert statements"? let's be consistent.
needs comma after "on".
grammar "will not be ran" is not right. Fix tenses.
just in case what?
Comment #286
jhodgdonI propose that I redo #285 in the form of a patch/interdiff. ;)
Comment #287
jhodgdonRegarding that, let me know if you are working on a new patch because it would be a pain to have to try to merge...
Comment #288
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedAfter 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
Comment #289
jhodgdonOK. 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!
Comment #290
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI'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.
Comment #291
jhodgdonYes, 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?
Comment #292
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOk, 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.
Comment #293
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #295
dawehnerJust 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.
Comment #296
yched CreditAttribution: yched commentedSorry 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) ?
Comment #297
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOk, 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.
Comment #299
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedAdded group to new test.
Comment #301
dawehnerAh yeah that is totally fine!
Do you mind mentioning that by default we throw exceptions when an assertion is not met?
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?
Comment #302
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedPrevious 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.
Comment #303
dawehnerThat is a totally good point. Thank you.
Comment #304
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedDoc changes only, no new code, so this should pass as well.
Comment #305
dawehnerI really like the current state of the patch. Here is just a list tiny nitpicks before an RTBC.
Nitpick: new line missing
nitpick: We use "\$class_name" instead inline
Comment #306
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThanks.
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.
Comment #307
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedPrevious 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.
Comment #308
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedK, 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.
Comment #309
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedOne 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:
We'd have
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?
Comment #310
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedNoted the PHP 5.x/7 behavior in the issue summary.
Comment #312
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commented306 failed as expected due to lack of rebase. 308 is current patch
Comment #313
Crell CreditAttribution: Crell at Palantir.net commentedA 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!
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. :-)
Non-blocker: Since we're on PHP 5.5, callable is a safe type to hint against for $callable. Let's do that.
Technically unnecessary, as PHPUnit will fail for us if the expected exception doesn't get thrown.
Comment #314
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThe 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.
Comment #316
Crell CreditAttribution: Crell at Palantir.net commentedUm, I wasn't suggesting it move in this patch... Just that a general composer package would be useful for others. :-)
Why rename the class?
Comment #317
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedWell, 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.
Comment #318
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #320
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI feel stupid.
Comment #321
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #322
dawehnerI totally agree, but this should not block us from moving forward here. We can always extract it later and then pull it in.
Comment #323
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, looks great to me now! :)
Comment #324
dawehner@Aki Tendo
Thank you so much for pushing that issue and reinventing it multiple times.
Comment #325
alexpottHere are some docs fixes that I think we should make - leaving at RTBC because it is only docs.
primarly
"They are used ensure that methods' return values are the documented datatypes." i think we need the possessive and to ensure not insure.
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.
therefore
Comment #326
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#325 makes sense to me, leaving at RTBC.
Comment #327
dawehner+1
Comment #328
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedRegarding #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.
Comment #330
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI 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.
Comment #331
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThis 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.
Comment #332
dawehnerYeah that is much clearer
Back to RTBC
Comment #333
alexpottCommitted 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).
Comment #335
Wim LeersAki 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) :)
Comment #336
twistor CreditAttribution: twistor commentedWoot! This is going to be awesome.
Comment #337
almaudoh CreditAttribution: almaudoh commentedAwesome!! Congratulations @Aki Tendo. This also happens to be one of the longest issues in recent times (and I just made it a comment longer :) ).
Comment #338
pfrenssenExcited to see this in! Thanks @Aki Tendo for the great work!
Comment #340
chx CreditAttribution: chx commentedSIGH. 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.
Comment #341
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedI'll draft up a full change record later today - I haven't written one before so I'll need someone to check over it.
Comment #342
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedDraft of change record prepared.
https://www.drupal.org/node/2569701
Comment #343
Crell CreditAttribution: Crell at Palantir.net commentedHoly 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.
Comment #344
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThen 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.
Comment #345
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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.
Comment #347
Wim Leershttps://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 again in #340 have been addressed.