Unhelpful PHP error with NULL type property

Added the test that will fail with such definition. It is practically useless, I think, but it shows that the input and expected data are not the same. If you try to render it, a PHP error occurs. Maybe we should throw an exception in that case, I don't know.

I propose to throw an exception if the types of the property contain values that are not strings, which will include floats and special YAML types. We definitely do not want to correct the value !!float 12.3 in any way. We should throw a meaningful exception, indicating the component ID and property name. Then developers can easily identify and fix the problem.

If we are going to fix NULL for developers automatically, we still have to check for other types and throw an exception. So it's just more code with basically the same result.

Added check for property type definition values, and if it contains a non-string value, throws an exception. In my case, it is now clear where the problem is:

Drupal\Core\Render\Component\Exception\InvalidComponentException: The component "foo:bar" uses non-string types for properties: required. in Drupal\Core\Theme\Component\ComponentValidator->validateDefinition() (line 96 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json
name: Bar
props:
  type: object
  properties:
    required:
      type: [null, boolean]
      title: Required

Tried to get some feedback in the slack channel #components but no luck.

Believe this is a net improvement for sure though

Test-only feature shows that

1) Drupal\Tests\Core\Theme\Component\ComponentValidatorTest::testValidateDefinitionInvalid with data set #3 (['https://git.drupalcode.org/pr...a.json', 'Call to Action', 'My description', ['object', ['text'], [[null, 'Title', 'The title for the cta', ['Press', 'Submit now']], ['string', 'URL', 'uri'], ['string', 'Target', ['', '_blank']], ['Drupal\Core\Template\Attribute', 'Attributes']]], 'my-cta', 'module', 'sdc_test:my-cta', [[[[]]]], '', 'sdc_test', 'my-cta.twig', 'my-group'])
Failed asserting that exception of type "Drupal\Core\Render\Component\Exception\InvalidComponentException" is thrown.
2) Drupal\Tests\Core\Theme\Component\ComponentValidatorTest::testValidateDefinitionInvalid with data set #4 (['https://git.drupalcode.org/pr...a.json', 'Call to Action', 'My description', ['object', ['text'], [[['string', null], 'Title', 'The title for the cta', ['Press', 'Submit now']], ['string', 'URL', 'uri'], ['string', 'Target', ['', '_blank']], ['Drupal\Core\Template\Attribute', 'Attributes']]], 'my-cta', 'module', 'sdc_test:my-cta', [[[[]]]], '', 'sdc_test', 'my-cta.twig', 'my-group'])
Failed asserting that exception of type "TypeError" matches expected exception "Drupal\Core\Render\Component\Exception\InvalidComponentException". Message was: "Drupal\Core\Theme\Component\ComponentValidator::Drupal\Core\Theme\Component\{closure}(): Argument #1 ($type) must be of type string, null given" at
/builds/issue/drupal-3462156/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php:278
/builds/issue/drupal-3462156/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php:84
/builds/issue/drupal-3462156/core/tests/Drupal/Tests/Core/Theme/Component/ComponentValidatorTest.php:62

I don't see that anyone has received the code for this issue, only that the test only feature has been used. Remember, issues do need a code review. I thought at first that this was throwing a new exception but I see that it is not. I do think a comment is needed, which I indicated in the MR, perhaps some of the detail in #4 would be useful there.

The title here should include some indication of what this fix applies to. The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful an concise. See List of issue fields → .

So, two small things, although this still needs a review by someone familiar with this component.

Added comment and adjusting issue title

Believe title reflects the change so removing that tag.

Also appears additional feedback has been addressed

I think the test could be slightly clearer (see MR comment), but I think this is ready nonetheless 👍

Changing to NW to implement the suggested changes to the dataprovider.

Updated MR and improved data provider.

P.S. Sorry for the delay, I did not receive notification from MR's comments.

 * @return array
   *   The data.

Shouldn't this be updated now too?

I have addressed the comment in #14 from @smustgrave.

believe feedback to be addressed

All questions are answered here. I didn't do a code review, I am a bit too tired.

Leaving at RTBC

larowlan → closed merge request !8807

Committed to 11.x - thanks folks.

Automatically closed - issue fixed for 2 weeks with no activity.

I'm running into an issue caused by this fix. In many components, I've allowed multiple types. Example:

  properties:
    is_active:
      type:
        - null
        - boolean

Now, I can't see how to do this. Furthermore, I've written code that's in prod that does this, but there's no change record. When prod updates to 11, this will break.

Hi, it should be:

  properties:
    is_active:
      type:
        - 'null'
        - boolean

Other types are basically cast to a string by the YML parser, and boolean becomes 'boolean' internally as any other type because they are both strings. However, the null type is special for YML, and it is not cast to anything. It just becomes a simple NULL. In fact, this has never worked, and your previous example should also have thrown an exception when you pass an actual NULL value.

Previously, if a component could actually have a null value dynamically, and was mostly used for a fail-safe solution (for example, a field with data was empty, but you expected it to be present in most cases), it would fail in production without a clear understanding of why. This was a much more dangerous approach, which I faced multiple times. Without other patches ( ✨ Unhelpful error message with enumeration problem Active ), you will not even know which component has failed on the page. It is just a WSOD.

As proposed initially, we could add some kind of protection by casting all null types to 'null' internally to improve the DX. Or we could simply add a note to the change record mentioning this. I am not sure which is better. I think we should get input from the core maintainers here.

From a component developer's perspective, the requirement to wrap null into a string seems unnecessary, because other data types do not require that. This is what the exception added in that issue is all about - it highlights the problems and errors in the component that were previously hidden until a NULL value was passed and it was confusing why the component failed when the type was clearly listed. At least now, it triggers an exception before the code is deployed, but it looks like it's not clear what's actually wrong. Maybe we should improve the exception message by adding extra information about null.

From the code developer's point of view, the component's validator is already difficult to understand, in my opinion. Adding such workarounds to automatically fix these issues will not solve the problem, and may even make it worse. Additionally, we are walking on a slippery slope, as there are likely other cases that are handled differently by the YML parser, and would require additional workarounds for those. I can't recall, but it's likely that if we try to work around this at the validator level it will be necessary in other parts of the component code, as the validator would only "fix it" for the validation process but it would still be null inside, and other parts of code might still fail. At the current stage, it detects problems very early and throws an exception, and then we don't worry about it much. It should either be fixed or deleted by the component's developer.

As another suggestion, we could add a best practice/tip/hint/guideline to wrap all values in strings. This would make it consistent and safer. Internally, component's code expects all type values to be wrapped in strings, so it makes sense to do this. However, this would require wrapping everything except null. This is what it actually should be from the code perspective:

  properties:
    is_active:
      type:
        - 'null'
        - 'boolean'

^ and this is the "proper" example and usage, based on how YML parser will treat it.