Add validation constraints to media.settings

Picking this up for DrupalCon Lille

media.settings:icon_base_uri is described as the 'Full URI to a folder where the media icons will be installed' from the media module schema YAML file.

It is set by configuration install YAML to public://media-icons/generic.

On hook_install() basically the files at core/modules/media/images/icons/ get copied into the actual directory pointed by the value of the config key, i.e. the four images there.
These icons are shown for instance on the /admin/content/media page, when relevant for the media type.

From grepping the configuration key, there does not seem to be any UI where this configuration can actually be changed.

$ git grep icon_base_uri
core/modules/media/config/install/media.settings.yml:icon_base_uri: 'public://media-icons/generic'
core/modules/media/config/schema/media.schema.yml:    icon_base_uri:
core/modules/media/media.install:  $destination = \Drupal::config('media.settings')->get('icon_base_uri');
core/modules/media/src/Annotation/MediaSource.php:   * 'media.settings.icon_base_uri'. When using custom icons, make sure the
core/modules/media/src/Entity/Media.php:    return \Drupal::config('media.settings')->get('icon_base_uri') . '/' . $default_thumbnail_filename;
core/modules/media/src/MediaSourceBase.php:        return $this->configFactory->get('media.settings')->get('icon_base_uri') . '/' . $default_thumbnail_filename;
core/modules/media/src/Plugin/media/Source/File.php:    $icon_base = $this->configFactory->get('media.settings')->get('icon_base_uri');
core/modules/media/tests/fixtures/update/media.php:      'icon_base_uri' => 'public://media-icons/generic',
core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php:    $icon_base = \Drupal::config('media.settings')->get('icon_base_uri');

In other words, this configuration key can only be changed externally, e.g via drush, or custom php, drush cset media.settings icon_base_uri 'public://media-icons/generic2'.

The hook_requirements() code seems to be the best reference of what is expected from the value.
It basically creates the directory dynamically and makes sure it is a directory and writeable.

This means that the actual value here is not expected to be a writable directory until its first use.
Checking for that may end up in a false positive, since the code actually does create it.

Based on this, the only condition I can think of for this configuration key is just to be a string that can become a writable directory.
And that, by itself, may not be such a reasonable custom constraint to create :sweat_smile:

Hence, I can only think of using the PrimitiveType constraint check here, but that should already be happening 📌 Add validation constraints to taxonomy.settings Needs review .
It is not clear if one should be added, or maybe we want to change config_inspector code to realize that there is already primitive type validation on simple types in the schema?

Conclusion: There is not really a strong case for validation of the media.settings:icon_base_uri configuration key.
Suggestions welcomed.

We could validate if the string is a stream wrapper? that it starts with something in the shape of [public|private]://[something]?
Or can you just add a normal path as well?

marvil07 → changed the visibility of the branch 3395585-add-validation-constraints to hidden.

We could validate if the string is a stream wrapper? that it starts with something in the shape of [public|private]://[something]?
Or can you just add a normal path as well?

@borisson_, that was an excellent discovery question 👍 .

In theory, a normal path can also be used.
The code at hook_requirements() provides logic for handling both a stream wrapper URI, and a local file system path.
Said that, I manually tested that, and it just does not work as expected.

In other words, yes, it seems like a stream wrapper URI is required, which means either (a) checking for it is worth, or (b) there is a bug for handling normal file system paths.
I am assuming (a) is the case.

New commits on new 3395585-validate-icon_base_uri topic branch and related new [MR #5675](https://git.drupalcode.org/project/drupal/-/merge_requests/5675) are the following.

- ed7e3057c2 Add a new StreamWrapperUri constraint plugin
- b338b94b1e Validate media.settings configuration icon_base_uri key for a valid stream wrapper URI
- 266daac448 Fix a few problem on the new constraint validator

I have changed the issue summary to include the new constraint as API change.
Also a related change record added for it at New config validation constraint: StreamWrapperUri → .

In other words, yes, it seems like a stream wrapper URI is required, which means either (a) checking for it is worth, or (b) there is a bug for handling normal file system paths.
I am assuming (a) is the case.

This is a good question, I'm not sure about this assumption, let's ask one of the media maintainers to be sure.

Mostly looks good to me but it certainly needs tests. A few questions came up as well.

@borisson_, @phenaproxima: thanks for the feedback 👍

I added the suggestions, and also added test coverage on this round.

BTW I started with a unit test, but then morphed into a kernel test, so I get the underlying service available.

New commits on 3395585-validate-icon_base_uri topic branch and related MR #5675

- 1275f421a1 Rephrase constraint message.
- 41f1d1dfca Use class name instead of machine name for service lookup
- b017bc580c Fix typo
- 9bb2b18701 Add test coverage for StreamWrapperUriConstraint
- 2f7dddc947 Here you go cspell

A few small things but this test coverage looks good to me! Once the nits are fixed, I'd be comfortable marking this RTBC.

@phenaproxima: thanks for the suggestions.
I have applied most of them directly.

The assertSame() change needed some extra context from translatable strings.

Back to NR.

I really like the new StreamWrapperUri Constraint, I think that one can be helpful in other schema's down the road as well.
Leaving for further review to @phenaproxima, but in my eyes this looks good to go.

Wondering if we need to do the elaborate translation stuff in the test - I think that might be a little on the brittle side. But if I'm wrong, I would not consider this blocking feedback.

@phenaproxima: added a change to avoid using StringTranslationTrait by removing added html tags.

I'm satisfied. :) Let's ship it!

The validation in place is not yet sufficient for the use case, I'm afraid 😅

@phenaproxima, @Wim Leers, thanks for the review 👍

I added a couple of changes to the 3395585-validate-icon_base_uri topic branch and MR.

- 411fc720d5 Validate the stream wrapper uri is a string at the start
- 715ef4bc1f Extend test validation to cover more stream wrappers

On the read-only stream wrapper case, I think it still may work, please find details in my reply at gitlab, and above.

Back to NR.

On the read-only stream wrapper case, I think it still may work, please find details in my reply at gitlab, and above.

I don't think so 😅

Responded in detail on the MR, with another example where we'll need more detailed validation 😊

@Wim Leers, thanks, I appreciate the feedback 👍

Extending StreamWrapperUri constraint

I have extended the constraint to be more flexible, and then used its new option on the original case targeted in this issue.
More details on my reply there.

New commits on 3395585-validate-icon_base_uri topic branch and related MR #5675:

- e39bf90e76 Require read and write stream wrapper flags on StreamWrapperUriConstraintValidator
- 508ab9f27f Add class constraint class restriction
- e8985374c9 Introduce constraint options for StreamWrapperUri constraint plugin
- 4fa3708d7d Require readable and writable stream wrapper on icon_base_uri media.settings option
- c6c058865a Fix typo
- 7aed3751e6 Fix logic error on constraint validator

Uncovering new configuration validation errors

In an unexpected turn of events, as the last gitlab CI pipeline documents, many new places are now failing validation while trying to install media module 😅.

Looking a bit into them, I see that the public:// stream wrapper is not available on them.
I am wondering why is that the case.
The public stream wrapper is declared at stream_wrapper.public service on core/core.services.yml file, which is not really part of any module.

I may get back into this, but any clues will be highly welcomed.

Will look at this as soon as I can! 😊

I really like the new Constraint here, we can probably reuse that in new places as well.

The latest test run has failing tests, correcting the status