Active
Project:
CDN
Version:
4.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
4 Jan 2023 at 16:59 UTC
Updated:
10 Mar 2023 at 09:59 UTC
Jump to comment: Most recent
Problem/Motivation
In CdnSettings.php, there are a few calls to the assert() function to verify the config. According to the PHP docs (https://www.php.net/assert), assert is only supposed to be used for debugging, not input validation. Assert can be basically turned off in php.ini, which could cause people to miss config problems that should be fixed.
Proposed resolution
Change the assert calls to something else. In buildLookupTable(), the asserts could possibly be changed to the ConfigValueException that is already used in that function. GetStreamWrappers() doesn't already use ConfigValueException and arguably isn't testing the config, but maybe it could use the same exception.
Issue fork cdn-3331104
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
nbanderson commentedI came across this later: https://www.drupal.org/node/2492225#s-configuration-checking. So maybe it's expected to use assert this way in Drupal, although the document notes that it's a gray area. It seems safer to throw exceptions though, if you really want someone to know about a problem.
Comment #3
wim leersThe CDN module is the very first module to use config validation — see #2164373-16: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …. So it's configuration should be valid.
See
/src/Plugin/Validation/Constraint/*.php.Unfortunately, Drupal's configuration system does not guarantee validity of configuration at all. It's totally possible for you to
drush cex, modifycdn.settings,drush cimand … tada, it's invalid, andCdnSettingswould crash while interpreting the configuration in obscure ways. Once #2414951: Validate configuration schema before importing configuration is fixed, I'll gladly remove these!The
assert()s are not essential. But on development environments, they will run, and will provide actually helpful error messages.So it's not being relied on, it's just a safety net! 😊
Did that answer all of your questions? 😊
Comment #4
nbanderson commentedHey, thanks for the reply. The config validation is definitely cool. I wasn't suggesting to remove the asserts though, just change them from an assert to an exception or error. The reason being that we had asserts turned off in our dev environment (although I'm not sure if that was some default we unexpectedly inherited or if it was intentionally done by someone to make dev more like prod). So we didn't realize that our config was failing one of the asserts until we started trying out a different dev environment. I know "accidentally had asserts off in dev" is a bit of an edge case, but I'm probably not the only person it's happened to, and changing the asserts to an exception or error would be more visible in those situations. Not a big deal either way, just something I thought might be a tiny developer experience improvement.
Comment #5
wim leersGood point! Now I'm wondering why I even implemented it this way 🤔
It looks like I had
assert()s in there prior to introducing config validation, and then I kept minimalassert()s around in #2969065: Use typed config validation constraints for validation of cdn.settings simple config. I agree with your reasoning though, so let's change it! 😊👍Comment #6
wim leersIn a very surprising turn of events, this is now blocking #3334243: CDN 4.x requires PHP >=8.1 but composer does not respect it due to bug in d.o composer facade, see #3334243-18: CDN 4.x requires PHP >=8.1 but composer does not respect it due to bug in d.o composer facade.