Problem/Motivation
Media entity used to provide custom validation API to be used by type plugins. In #2529178: Convert media type validation code to a constraint we converted this feature to core's Constraint API. All media type providers need to follow that change.
Proposed resolution
Media entity Instagram should follow this change too. Media entity Twitter already did that in #2578927: [Chase media_entity] Convert validation to Constraints API and can be used as a reference.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | validation-to-constraints-2578935-9.patch | 9.16 KB | sdstyles |
| #6 | validation-to-constraints-2578935-6.patch | 9.14 KB | sdstyles |
| #4 | validation-to-constraints-2578935-4.patch | 9.02 KB | sdstyles |
| #2 | validation-to-constraints-2578935-2.patch | 2.84 KB | sdstyles |
Comments
Comment #2
sdstyles commentedComment #3
slashrsm commentedThank you. I can one comment:
The actual check should be done in constraint validator. attachConstraints() is only adding them without assuming what result of the check will be.
See how we did it in media_entity_twitter: https://github.com/drupal-media/media_entity_twitter/pull/9/files
Would also be great to have tests coverage for constraints (Twitter pull request implemented that too and can be used as a reference).
Comment #4
sdstyles commentedChanged, also added tests coverage as on Twitter pull request.
Comment #5
slashrsm commentedGreat. Looks almost ready to go. Just few nit-piks:
s/media_entity_twitter/media_entity_instagram
This should be static if we want to use it as constraint validator currently does. We took a different approach with Twitter and exposed only regexp.
Both of this seem to be wrong URL. ID parts seems OK in both cases.
Comment #6
sdstyles commentedExposed $validationRegexp property in constraint validator.
Comment #7
slashrsm commentedThanks! Created pull request on GitHub to trigger Travis build. Will merge when it confirms it is green.
https://github.com/drupal-media/media_entity_instagram/pull/10
@sdstyles: We usually attach inderdiffs to update patches. See documentation for more info: https://www.drupal.org/documentation/git/interdiff
Comment #8
slashrsm commentedIt failed :(
https://travis-ci.org/drupal-media/media_entity_instagram/jobs/84265418
Namespace is wrong. Same with constraint validator.
Comment #9
sdstyles commentedChanged namespace in InstagramEmbedCodeConstraintValidator.php to
Drupal\media_entity_instagram\Plugin\Validation\ConstraintValidator;
Comment #10
slashrsm commentedSame problem with InstagramEmbedCodeConstraint itself. You can try to run test locally to see if it passes. It should run pretty quick.
Comment #11
sdstyles commentedHi @slashrsm, I've created new pull request where I fixed namespaces and another small issues, please review it.
https://github.com/drupal-media/media_entity_instagram/pull/11
Comment #12
slashrsm commentedMerged. Thanks!