Add validation constraints to image.settings

The preview_image key on image.settings configuration stores a file path.
Symfony already provides an Image validator, that checks the file exists and its mime type is an image.
The change on the MR adds that validation.

I don’t think using \Symfony\Component\Validator\Constraints\ImageValidator is appropriate here. That leverages on GD and PHP getimagesize functions, ehich would fit with core’s GD image toolkit. But other toolkits could provide support for other image formats.

IMHO the validation here should be based on each site’s active toolkit.

@mondrake,

I don’t think using \Symfony\Component\Validator\Constraints\ImageValidator is appropriate here. That leverages on GD and PHP getimagesize functions, ehich would fit with core’s GD image toolkit. But other toolkits could provide support for other image formats.

IMHO the validation here should be based on each site’s active toolkit.

Yes, symfony's ImageValidator uses GD, but that applies when it receives the parameters to validate sizes.
No parameter is passed, so it is mostly used to validate the image mime type.
As for GD, it should be available.

So, yes a custom validator can be created to use drupal image toolkit abstraction, but that may be more useful for other configuration validation cases.
In this case, it is mainly about checking that the file exists, and it is an image, which the image constraint does by extending the File constraint.
Tangentially, I think this configuration option does not even have a UI form available to be set, so tailoring to the currently used image toolkit may be more than is needed here.

Seems my comment didn't make it before. But failure seems legit.

Seems my comment didn't make it before. But failure seems legit.

@smustgrave, thanks for pointing it out.

Indeed, the test was failing.
Even if the error is quite obscure, the problem is related to the new configuration validation being triggered.

After a lot of debugging, the problem appears clearly: the failing test is testing image settings migration from the drupal 7 fixture, which has an invalid/non-existent image file pointed on the preview image related configuration, for which configuration validation was added.

The test fix is to point the test to use an image that actually exists, instead of an invalid image path.

On one end, it is great that validation is triggered and checks this, but I was amazed by the obscure and generic database connection object non-serialize-able exception, that I had not encountered before.
It would be nice to improve that in some way, but that is definitely out of scope here.

Hopefully the test run confirms what I see locally.

Will see if someone more familiar with schema system can see. But changing the migration tests seems like it would be out of scope?

Will see if someone more familiar with schema system can see. But changing the migration tests seems like it would be out of scope?

Well, not really, there was a configuration that was wrong before, and now we fixed it. In the required keys issue we had to ignore some test failures, because we had thousands of places where configuration was invalid in the tests.
I think we're going to see this more often.

Ah gotcha.

This is not yet ready, because there is an impact on the public API surface that is A) not documented in the issue summary nor change record, B) needs some additional scrutiny, because it is currently on a clear path to confusion 😅

@Wim Leers, thanks for the review! :+1:

The constraint is now renamed on the merge request.
I did not take any action around parameters, e.g. to prevent trying to use the validator options.
I think that is OK for now.

This is not yet ready, because there is an impact on the public API surface that is A) not documented in the issue summary nor change record, B) needs some additional scrutiny, because it is currently on a clear path to confusion

I have just added a note on the issue summary.
I will wait for review feedback before opening a change record.

With my limited knowledge of the schema system this looks correct to me? My question though @Wim do we need CRs for schema changes?

Yes, we do need new CRs for new validation constraints. Recent examples: https://www.drupal.org/node/3383114 → , https://www.drupal.org/node/3372085 → , https://www.drupal.org/node/3376195 → .

@smustgrave, @Wim Leers: thanks for the feedback.

The change record has now been created at https://www.drupal.org/node/3400191 → , back to NR.

Ah that was my bad @wim. CR looks good though.

I think it's reasonable to RTBC/commit this without test coverage because this is tested upstream, by Symfony 👍

I read the issue summary and commented. I didn't find any unanswered questions or other work.

The change to the Drupal7 migration fixture caught my eye. I wanted to know if we should make the same change in any other drupal7 migration fixture in core with that line. In turns out that both the forum and tracker modules have a D7 fixture with the same line. But those fixtures aren't used for testing image settings and are unlikely to ever do so. So, they don't need to be changed.

I meant to say thanks for the easy to follow issue summary.

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

Regarding #8. There is an issue for that 🐛 Exception trace cannot be serialized because of closure Needs work and it is a real nuisance when working on migration issues. The work around I use is composer require mpyw/phpunit-patch-serializable-comparison.

With this change, it looks like image.settings has constraints on all of its properties. So it should have the FullyValidatable constraint as well :)

@phenaproxima++

This now indeed needs the FullyValidatable constraint to be added to image.settings. See https://www.drupal.org/node/3404425 → .

vakulrai → changed the visibility of the branch 3395616-img-settings-config-validation-constraint to hidden.

I have added the constraint validators to the core/modules/image/config/schema/image.schema.yml , please review.

Thanks !

@vakulrai: that looks fine, thanks!

Next up: all the other points of feedback (on the MR itself).

Hi @wim-leers i tried to add my comments on the pending feedback , please review.

Points covered :

• https://git.drupalcode.org/project/drupal/-/merge_requests/5132#note_255633
• https://git.drupalcode.org/project/drupal/-/merge_requests/5132#note_255644

Thanks !

🏓

This issue can be closed as duplicate of ✨ Add validation constraints to image.settings Fixed which is already fixed.