Current validation occurs in the FieldWidget object, but it should be in a Constraint level, so REST request are validated too (POST, PATCH).

CommentFileSizeAuthor
#9 video_embed_field-add_constraint-2844338-9.patch7.39 KBsam152
#7 video_embed_field-add_constraint-2844338-7.patch7.36 KBsam152
#7 interdiff.txt2.77 KBsam152
#6 video_embed_field-add_constraint-2844338-6.patch6.06 KBruloweb
#6 diff-2-6.txt3.55 KBruloweb
#2 video_embed_field-add_constraint-2844338-2.patch5.17 KBruloweb

Comments

ruloweb created an issue. See original summary.

ruloweb’s picture

ruloweb commented
Status: Active » Needs review
StatusFileSize
new video_embed_field-add_constraint-2844338-2.patch5.17 KB

This patch requires to clear the cache, so Drupal detects the new Constraint Plugin.

sam152’s picture

sam152 commented
Status: Needs review » Needs work
Issue tags: +Needs tests

This is a great issue! Thanks for working on this. This will likely fail a test and should have a test of its own. 

  1. +++ b/src/Plugin/Validation/Constraint/VideoEmbedValidationConstraint.php
@@ -0,0 +1,24 @@
+* Video providers.

    Can we explain what the constraint does here? 

  2. +++ b/src/Plugin/Validation/Constraint/VideoEmbedValidationConstraintValidator.php
@@ -0,0 +1,57 @@
+   * @param \Drupal\video_embed_field\ProviderManager $provider_manager

    Missing description of the param. 

  3. +++ b/src/Plugin/Validation/Constraint/VideoEmbedValidationConstraintValidator.php
@@ -0,0 +1,57 @@
+  public function __construct(ProviderManager $provider_manager) {

    Should hint the interface. 

  4. +++ b/src/Plugin/Validation/Constraint/VideoEmbedValidationConstraintValidator.php
@@ -0,0 +1,57 @@
+      $this->context->addViolation($constraint->message, array('%url' => $value));

    should be short array syntax to match the rest of the module. 

  5. +++ b/src/Plugin/Validation/Constraint/VideoEmbedValidationConstraint.php
@@ -0,0 +1,24 @@
+  public $message = 'Could not find a video provider to handle the URL %url.';

    I don't think the URL belongs in the error. Should be the same as the other error message.

ruloweb’s picture

ruloweb commented

Thanks @Sam152 for the quick response :)

1) "Validation constraint for video embed fields which checks for existing providers."
2) "ProviderManager service."
3) Do you mean ProviderManagerInterface ?
4) Won't be necessary if the URL is not in the error message.
5) "Could not find a video provider to handle the given URL"

As soon as I get your thoughts I will create a new patch.

I also found an issue with the video_embed_wysiwyg module, I should replace:

--- a/modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php
+++ b/modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php
@@ -140,10 +140,11 @@ class VideoEmbedDialog extends FormBase {
    * {@inheritdoc}
    */
   public function validateForm(array &$form, FormStateInterface $form_state) {
    $provider = $this->getProvider($form_state->getValue('video_url'));
     // Display an error if no provider can be loaded for this video.
     if (FALSE == $provider) {
-      $form_state->setError($form['video_url'], VideoTextfield::getProviderErrorMessage());
+      $form_state->setError($form['video_url'], $this->t('Could not find a video provider to handle the given URL.'));
       return;
     }
   }
sam152’s picture

sam152 commented

Good on all counts.

ruloweb’s picture

ruloweb commented
Status: Needs work » Needs review
StatusFileSize
new diff-2-6.txt3.55 KB
new video_embed_field-add_constraint-2844338-6.patch6.06 KB

New version which applies @Sam152 recommendations.

Still missing the test. will be uploaded this days :).

Let's test bot this.

sam152’s picture

sam152 commented
StatusFileSize
new interdiff.txt2.77 KB
new video_embed_field-add_constraint-2844338-7.patch7.36 KB

With test and minor style changes.

Status: Needs review » Needs work

The last submitted patch, 7: video_embed_field-add_constraint-2844338-7.patch, failed testing.

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new video_embed_field-add_constraint-2844338-9.patch7.39 KB

Missing group annotation on test.

  • Sam152 committed 418b6e9 on 8.x-1.x authored by ruloweb 
    Issue #2844338 by ruloweb, Sam152: Replace field widget validation for a...
sam152’s picture

sam152 commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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