Settings to disable entity validation

Besides the failed test, patch #8 woks for me using Drupal 9.5.3, Feeds 8.x-3.0-beta3 and PHP 8.1.6

I think this issue requires some clarification, on what kind of approach to use.
It makes sense to configure skipping validation per field type, however, what if the user wants to skip all possible validations?
I'd like to have such a checkbox/option and when it is enabled show the skip validation types field and specify what kind of rules to apply.

@Andriy Khomych
I've tried to make the next steps more clear in the issue summary.

@MegaChriz, thank you, looks perfect, I'll see if I can help with this.

@MegaChriz, I'll work on this issue and prepare MR soon.

@MegaChriz, opened MR - https://git.drupalcode.org/project/feeds/-/merge_requests/119

I see some possible downsides:

• The feeds configuration and options will save in the configuration all constraints, even when they are disabled, and mark them with a 0 value. It complicated one part of the code and it was required to filter out such values.
• I'm not sure if we should provide hook updates for users who used the patch with `skip_validation_types` configuration. IMHO - they can visit the feed type configuration, enable the extra checkbox, and export the configuration to ensure that it is working correctly.

The last submitted patch, 16: feeds-add-settings-to-bypass-validation-3062232-beta2.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Andriy Khomych → closed merge request !119

Andriy Khomych → closed merge request !120

Sorry for spamming, closed all other MRs and fixed the issue with tests here.

https://git.drupalcode.org/project/feeds/-/merge_requests/121

Duplicated:

I see some possible downsides:

• The feeds configuration and options will save in the configuration all constraints, even when they are disabled, and mark them with a 0 value. It complicated one part of the code and it was required to filter out such values.
• I'm not sure if we should provide hook updates for users who used the patch with `skip_validation_types` configuration. IMHO - they can visit the feed type configuration, enable the extra checkbox, and export the configuration to ensure that it is working correctly.

@Andriy Khomych
Thanks for working this! I scanned the code and at first glance it looks good. Kudos on adding test coverage too! I'll ask on the Feeds meeting this week if someone wants to test it.

The feeds configuration and options will save in the configuration all constraints, even when they are disabled, and mark them with a 0 value. It complicated one part of the code and it was required to filter out such values.

I think this is fixable by only saving the enabled options: in the submit handler, manipulate the form values instead of saving the form values to the feed type as is. If I'm not mistaken, I think it can be done with a array_filter() call, or else with a foreach loop.

I'm not sure if we should provide hook updates for users who used the patch with `skip_validation_types` configuration. IMHO - they can visit the feed type configuration, enable the extra checkbox, and export the configuration to ensure that it is working correctly.

I think that an update function for users that used a patch is not a requirement. In my opinion, update functions are usually used to update configuration or data that came with an official release. I did have made an exception to this in the past, in a situation where it was complicated to manually update from a patched version to the official released version.

@MegaChriz, thank you for the quick check.
Regarding:

I think this is fixable by only saving the enabled options: in the submit handler, manipulate the form values instead of saving the form values to the feed type as is. If I'm not mistaken, I think it can be done with a array_filter() call, or else with a foreach loop.

I was thinking the same, but it looked like I should implement some custom exception in such a case, so, I skipped it.
Well, if someone can update it as well - that would be great, otherwise, I can try to do it later.

Hm, I wonder what you mean with custom exception? But I haven't studied the code in detail yet.

Just to be sure that we're talking about the same thing, I think that the following:

foo: 0
bar: bar
baz: baz
qux: 0
thud: thud

should be transformed into:

- bar
- baz
- thud

Hm, I wonder what you mean with custom exception? But I haven't studied the code in detail yet.

Sorry for the unclear comment, was in a hurry.
I meant that I don't like the idea to create specific if conditions and it was like this for the submit handler.

should be transformed into:

- bar
- baz
- thud

Exactly!

Well, I decided to improve the config save.
@MegaChriz thank you for pointing me in the right direction.

I've taken a look at the code and made some remarks. :)

One test I would add is for ignoring a required field only.

I still have to try out the code in action though, so you could wait for that if you want to make adjustments.

Thank you MegaChriz.
I'll try to improve it.

I tested the MR in the UI and I noticed the following things:

1. In the list of validation types I see some that don't look useful in some contexts, such as the constraint "UserName" when you are importing nodes. However, when inspecting the constraint definitions, it doesn't look like you can filter ones out based on context. So I guess we need to leave this as is.
2. I noticed that on the feed type settings that when you later turn off "Skip validation", the validation types that were selected remain still selected when saving the feed type.

   Steps

   1. On a feed type, check "Skip validation".
   2. Select a few validation types, for example "NotNull" and "Length".
   3. Save the feed type.
   4. When the config gets exported to YAML, you'll now have the following:
      

        skip_validation: true
        skip_validation_types:
          Length: Length
          NotNull: NotNull
      

      This is good.

   5. Now uncheck "Skip validation" on the feed type and save again.
   6. In the config there is now this:
      

        skip_validation: false
        skip_validation_types:
          Length: Length
          NotNull: NotNull
      

      The validation types remain selected. I think this is not good.

3. Next I tried to import data without providing a value for a required field.
   • (OK) With "Skip validation" disabled, the import failed with the error "This value should not be null." for the field. This is good.
   • (OK) With "Skip validation" enabled, the import succeeded. This is good, validation gets skipped.
   • (OK) With "Skip validation" enabled and only select the type "Length", the import failed again. This is good, validation for required fields should not be skipped.
   • (OK) With "Skip validation" and having selected types "Length" and "NotNull", import succeeded again. This is also good.
   • (x) Now I disabled "Skip validation" (but types "Length" and "NotNull" were still being saved on the feed type, see #2) and the import still succeeded. This is not good. The import should have failed because I configured the feed type to not skip validations.

If you agree with point 2 that when unchecking "Skip validation", validation types should no longer be saved, it would be good to also have functional tests that covers saving the config. So that would lead to the following test cases:

1. Check "Skip validation" and save the feed type, assert that "skip_validation" is true and that "skip_validation_types" is empty.
2. Check "Skip validation", select some types and save the feed type, assert that "skip_validation" is true and that "skip_validation_types" has the expected values.
3. Check "Skip validation", select some types and save the feed type. Then edit the feed type, uncheck "Skip validation" and save the feed type again. Assert that "skip_validation" is false and that "skip_validation_types" is empty.

Thank you MegaChriz for the feedback, agree with point 2 and tests for it.

Hey MegaChriz, feel free to review it.
As for

If you agree with point 2 that when unchecking "Skip validation", validation types should no longer be saved, it would be good to also have functional tests that covers saving the config. So that would lead to the following test cases:

I've added a separate test file FeedTypeSkipValidationTest.php, I guess no need to add them in the FeedTypeTest.php

In case someone needs, provided patch from MR.

The last submitted patch, 42: feeds-add-settings-to-bypass-validation-3062232-42.patch.patch, failed testing. View results →

MegaChriz, unfortunately, latest feed type form skip validation tests are failing.
I don't know why, on my local env they are passing.

With tests now passing, I think this is ready for review? Maybe I can find the time to do that review soon.

@Andriy Khomych
Thanks for your work! I've looked at the code and I made the following changes to it:

• The validation types are saved as an unordered list, thus as follows:
  

    skip_validation_types:
      - ConfigExists
      - Count
  

  Instead of:
  skip_validation_types:
  ConfigExists:ConfigExists
  Count:Count

I think that the first one looks cleaner.

• I moved the test FeedTypeSkipValidationTest to the Form directory and renamed it to FeedTypeFormSkipValidationTest. I think that makes it more clear it testing a form specifically.
• From FeedTypeSkipValidationTest:

      // Creates a new feed type for further editions.
      $feed_type = $this->createFeedType([
        'label' => 'Test Feed Type Skip Validation',
        'id' => 'test_feed_type_skip_val_one',
        'description' => 'A test for feed type creation.',
        'help' => 'A help text for this feed type.',
        'fetcher' => 'http',
        'fetcher_configuration[auto_detect_feeds]' => TRUE,
        'processor_configuration[insert_new]' => ProcessorInterface::INSERT_NEW,
        'processor_configuration[update_existing]' => ProcessorInterface::UPDATE_EXISTING,
        'processor_configuration[expire]' => 3600,
        'processor_configuration[authorize]' => FALSE,
        'processor_configuration[skip_validation]' => FALSE,
      ]);
  

  Here is an error in creating the feed type. The feed type is created programmatically. 'processor_configuration[skip_validation]' is used when supplying data for a form to submit. When using the API you have to use actual arrays, thus like this:

  // Creates a new feed type for further editions.
  $feed_type = $this->createFeedType([
    'processor_configuration' => [
      'skip_validation' => FALSE,
    ],
  ]);
  

• In FeedTypeFormSkipValidationTest I removed lines from creating a feed type and for submitting a feed type form that didn't look necessary for the test specifically. I left 'processor_configuration[owner_id]' => 'admin (1)', in because else you get you an error about that user 0 cannot be referenced. I don't know why this happens. Looks like a bug in the test setup.
• In FeedTypeFormSkipValidationTest::testEnableDisableSkipValidationSkipTypes I started the code with existing 'skip validation' configuration. This we can check if configuration doesn't change when not touching these fields.
• In SkipValidationTest, instead of needing two CSV files with the same content, I used the upload fetcher for the test instead. This did require adding a new test API method called copyResourceFileToDir() added to FeedsCommonTrait. That method copies a file to the public file directory that then can act as the source of a feed.
• SkipValidationTest was expanded a bit to check for more error messages.
• SkipValidationTest::testSkipValidationTypesLength() now also checks that the validation type "NotNull" is not skipped when you configure to only skip the "Length" validation type. The CSV source need to be adjusted a bit for that and as a result also the messages that were checked during all SkipValidationTest tests.
• I left out columns in content-skip-validation.csv that were not used for the test, to make it more easier to see what is relevant for the tests.

Maybe @irinaz wants to test this in the coming week. Otherwise I plan to commit it this Thursday (if tests still pass).

@MegaChriz, thank you, makes sense to me!
Appreciate your help.

I created DrupalPod, set body of article to be a required field, and checked box "skip validation". Items were imported without body. This DrupalPod can be use for more through testing.

• DrupalPod https://gitpod.io#snapshot/aea3a9aa-860b-491f-a302-ff4c7ea4666f

Thanks all for contributing! I merged the code!

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

This feature is very helpful to me. It would be great to get a release some time soon. Thanks.

@lelkneralfaro
Yes, I hope to create a new release this month. I originally planned to release in January this year, but other things got in the way. I'm now working on the last bits for the new release.