Move exceptions in \Drupal\Core\Entity\EntityDisplayBase::__construct to assertions

1.
assert('isset($values[\'targetEntityType\']) && isset($values[\'bundle\'])', 'Missing required properties for an EntityDisplay entity.');

I recommend not only asserting these things are set but that they are of the expected types, so check for is_string()

2.

+    assert('isset($this->pluginManager)', 'Missing plugin manager.');
+    assert('isset($this->displayContext)', 'Missing display context type.');

I'd recommend using an instanceof check against the expected interface or class for these two. Also, they are preconditions and should be in the opening assertion block after the parameter assertions. The error message should point out that these need to be set by the child before parent::__construct() is called.

I'm curious whether we have tests ...

Much of the time unit testing a runtime assertion is like unit testing a unit test - redundant. There will be corner cases where it needs to occur, like assertions that outright need their own method like the cache context tags validation, but those will be uncommon.

Existing unit tests on this code looking for the InvalidArgument exception are going to fail, and if they exist they will need to start looking for AssertionError. We need to get #2536560: Runtime Assertion unit and functional testing cleared as soon as possible for this reason. Also, the online testers are currently running tests with assertions off because of the .htaccess change in #2408013: Adding Assertions to Drupal - Test Tools. but the runtime assertions patch will fix this issue as well (since they are tied). It's currently passing in PHP 5.x, I don't know what's up with the 219 failures seen under PHP 7.

#4 summarizes how I feel about unit testing these assertions, if we have assertions that need testing for I wonder if they need to be assertions in the first place. In this case it looks like there are no existing unit tests for these exceptions as the patch is green.

As to testing the assertions: with assert() statements we're asserting that the statement evaluates to TRUE, so if it evaluates to FALSE or has a syntax error, it'll be caught by the online test runner (as soon as it runs with assertions on). But if the statement in the assertion is faulty (leading to a false positive), that's another problem which can't really be caught by a unit test anyway I think?

Which makes me wonder if we need to postpone any issues that introduce assertions (such as this issue) on the test runner running with assertions on first (i.e. #2536560: Runtime Assertion unit and functional testing or something more limited).

@Aki Tendo, seems the online test runners already run with assertions on?

Assert.active is the PHP default, but the web tests obey the setting in the .htaccess file. So until changed I'm pretty sure the web tests are picking up that setting. I don't know how the online test runners are handling the phpunits though.

Regarding #8

+    assert('isset($this->pluginManager)', 'Missing plugin manager. The plugin manager must be set before parent::__construct() is called.');
+    assert('isset($this->displayContext)', 'Missing display context type. The display context type must be set before parent::__construct() is called.');
+    assert('$this->pluginManager instanceof \Drupal\Component\Plugin\PluginManagerInterface');
+    assert('is_string($this->displayContext)');

Almost there. Checking isset() here is redundant. NULL (which fails isset() ) will fail both the is_string and instanceof checks. The message strings can be moved to the type checks, and the plugin manager error might be revised to state "set with an instance of PluginManagerInterface" or some such.

That reminds me of one of my minor peeves with PHP. isset() doesn't do exactly what it says on the tin - a variable that is set to NULL fails isset(). Since displayContext and pluginManager are declared properties of the class they are, strictly speaking, set, but they are set to NULL since no other value was given. I believe this has to do with how PHP internals works - all variables not declared have a value of NULL if referenced in a non-setting context the first time PHP encounters them. As a result of this I personally try to avoid using isset() as an assertion (or for any other check), instead checking for the expected datatype as much as possible.

It's a bit of extra work - but to be sure assertions are being eval'ed change the assert.active flag in .htaccess to 1 and submit that as part of the next patch. If the unit tests pass in that mode then the patch can be re-rolled with the .htaccess change pulled off. This workaround will allow this ticket to be tested and completed without regard to the status of #2536560: Runtime Assertion unit and functional testing. The other part of that ticket, registering an assert callback to throw AssertionError, isn't needed here.

Well in #5 assertions are definitely evaluated, as they were making the patch go red. I'm not sure why they would be evaluated in one test run and not in another but maybe some infra people can confirm.

Just as a development aid (as opposed to as a sanity check), wouldn't it make sense to do a separate isset() check here though, as the isset() checks have separate helpful error messages? I get that is_string() implies isset() && is_string(), but when that assertion fails it's up to the developer to find out whether the problem is that the variable is not a string or just hasn't been set.. whereas if we keep the check it's immediately clear that it's the latter.

I see runtime assertions as a means to point the way, not necessarily explicitly diagnose the problem to that detail. Whether with xdebug or a well placed var_dump() they'll be able to determine what to do next if it doesn't occur to them whether they failed to set it or set it to something invalid. Meanwhile over time assertions will add up I'd like to avoid having the system crawl when they are enabled. I'm also concerned with having the precondition lists for all the functions becoming huge - if we check isset() here we should check it everywhere to be consistent.

Aside from those concerns, PHP Type hinting does disambiguate between missing argument and wrong datatype, so it can't hurt except for the very rare corner case where the developer tries to set the item to null - isset() will be failed which to someone unfamiliar with PHP may be counter-intuitive.

By the way, $this->displayContext = ''; legal? If not an !is_empty() assertion is needed. If there is limit to the character ranges allowed in that string a preg_match() should be used.

added !empty() assertion

...with a character range check. No limitation had been defined so far, but it can't hurt to limit it to alphanumeric characters only as it's a machine name such as 'display' or 'form'

Is this a cache display context? If so we might want to place a static function in the CacheContext manager that holds that regex pattern so that the assertion of the pattern can be kept consistent from the various points in code that need to call it.

I dont think the displayContext in this class and the cache display context are the same...

Forgot a slash in the regex. The display context is set as a property in the extending classes (ie. EntityFormDisplay, EntityViewDisplay) so in core I believe it's just form or view for now. But I don't know that we should be hardcoding those in an assertion, couldn't new display contexts be defined outside of drupal core?

That would be a question for catch or alexpott. New display contexts can be displayed, but I believe they would be constrained to the limits of other machine names - if so the _ character needs to be included in the regex. Depending on their feedback on the regex this should be good to go.

Yes, limiting this to the regular machine name limitations for consistency makes sense, no reason to avoid underscores.

I like this use of assertions as it allows for a more precise definition of what we expect the data to look like than what we might write in a docblock.

I don't think the decision in asked for in #19 is a framework manager decision - this type of thing can be decided by a sub system maintainer. So one of the Entity or Field system maintainers in MAINTAINERS.txt

Config entities in yaml files - yes, use assert. From the admin panel - never. Assert cannot be used to check anything changeable by any user - including and especially the superuser.

The rule of thumb - can you do it from the browser? Yes - Normal control structures required. No - that is if a bad call requires the edit of a PHP or YAML file, assert is fine.

I'm not familiar enough with this code to make this call.

EDIT - If the codes could be coming from YAML files or admin control panel settings then assert() isn't appropriate unless the codes are validated at the time they are gathered from the admin panel. Such validation must occur - but it is also outside the scope of many internal functions. Since we are introducing assertions to an existing system extreme caution is needed in switching things over - we have to be sure that all user inputs are validated by other functions elsewhere in the system before they reach a function that is only asserting the state of an argument, not testing it.

Though assertions are a testing layer, they are only temporary and only present in development. A true hard test of a condition requires normal control structures since they are always present. Assertions are not tests in this sense.

Wonder with D10 if this is still needed?

Since there hasn't been a follow up or activity in 8+ years going to close for now.

If still valid please reopen, maybe updating issue summary.

Thanks all