Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EDTF validation constraint #78

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

adam-vessey
Copy link
Contributor

GitHub Issue: Islandora/documentation#1859

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

What does this Pull Request do?

Add a validation constraint to the EDTF field type, to ensure stored values pass basic validation.

What's new?

The validation constraint for EDTF fields.

  • Does this change require documentation to be updated? Unknown.
  • Does this change add any new dependencies? No.
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? Potentially, if something is expecting to be able to write non-EDTF values to EDTF fields; however... this seems like it would be a bad idea to support?

How should this be tested?

Could be done a couple of ways... with the code deployed:

  1. Run a migration containing both valid and invalid EDTF values, with validation enabled, the valid things should work, and those invalid should fail; or,
  2. Comment out the validation from the widget, and attempt to save with various values both valid and invalid EDTF values.

Additional Notes:

The settings from the widget are presently ignored by the constraint implementation, as there's not really a nice way to allow for later widening conversions of fields if desired... so the constraint allows both intervals and sets; however, it does not perform the "strict" validation, due to other issues with it...

... "strict" validation presently attempts to parse values into a DateTime object, which will fail with valid EDTF values such as:

  • 2021-21 (to mean "Spring, 2021")
    • Parsing into a DateTime fails, due to attempting to use the "21st" month
  • 19XX-12-31 (to mean "December 31st at some point in the 1900s", kind of thing)
    • "19XX" doesn't work as a year with DateTime

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/8-x-committers

@DonRichards
Copy link
Member

Note sure if this would help but this should have the "bad" characters I think https://www.w3.org/2001/06/utf-8-wrong/UTF-8-test.html

@rosiel
Copy link
Member

rosiel commented Feb 24, 2022

I tested with dates of '2021-01-01-01' and 'France' which fail the widget's validation but can be entered through Migrate. However even with this code in place,my migrations (migrate islandora csv) still seem to let them in to the date (EDTF) fields. Is there anything I have to do to "enable validation" other than have this code in place?

I tried setting validate: true in the destination setting of the yaml per this issue, but still the same result.

@adam-vessey
Copy link
Contributor Author

@rosiel :

Nah, that should do it; however:

Is there anything I have to do to "enable validation" other than have this code in place?

... "in place", would mean in the active configuration, and not just in whatever config entity in code in config/install in whatever module. So... might have to either:

  • do a partial installation of the config for the module, to get it into active config
  • reinstall whatever module (which should cause those configs to be reinstalled)
  • export your sites configs, make the changes to the relevant config entity in the export, and then import the bundle

... something of an aside, but: I see those "example" migrations claiming to be in the migrate_islandora_csv migration group; however, there's no migrate_plus.migration_group.migrate_islandora_csv entity to represent the group... anyway.

... a second aside, but: I've taken to start avoiding using migrate_plus.migration.* entities. Migration YAML can be placed in a module's /migrations directory, so for example, instead of /config/install/migrate_plus.migration.example_migration_node.yml, to instead be something like /migrations/example_migration_node.yml... then it's no longer necessary to have the dependencies/enforced/module shenanigans in place to have 'em be associated with the module... and then it is possible to have changes picked up just by changing the YAML (and possibly/probably running a cache clear/drush cr kind of thing?).

@adam-vessey
Copy link
Contributor Author

@DonRichards: Input encoding validation is well beyond the scope of this... would be up to whatever mechanisms that are accepting input (such as the forms, or CSV parsers, or whatever) to ensure that things are encoded/decoded appropriately.

@rosiel
Copy link
Member

rosiel commented Feb 24, 2022

Thanks @adam-vessey . When I added the validate:true line to the yaml, I did it through the config sync export/import route. It appears to be reading my changes to the spreadsheet file in the data directory, so that's good.
The files that you edited are PHP, so if I understand Drupal correctly they should be automatically read...

(oh, i didn't cache clear. Turns out you need to cache clear.)

OK, success - the three ones I messed with did not import. Is there any message anywhere about why they failed and which ones they were?

@adam-vessey
Copy link
Contributor Author

adam-vessey commented Feb 24, 2022

@rosiel: There's a few ways to see 'em:

  • migrate_tools GUI should make 'em available at something like: http://yourdomain/admin/structure/migrate/manage/migrate_islandora_csv/migrations/example_migration_node/messages (however, this might not work when/if things are moved over to non-entity migrations?)
  • via the CLI: drush migrate:messages example_migration_node should dump out things.
  • via the DB itself, looking at the contents of the migrate_messages_example_migration_node table... more generally, the migrate_messages_{migration_id} table.

However, one thing to note: These tables are wiped with each invocation of the migration... so if you run the migration and it dumps whatever errors in the messages table, but then try to run the migration again, the error messages would be lost.

@rosiel
Copy link
Member

rosiel commented Feb 25, 2022

OK, I'm still not having any luck finding the messages. The GUI shows 0 messages for the partial migration with some validation errors, and I am not sure how to use Drush on the ISLE instance I'm testing with. I'm not so hung up on messages as to think we shouldn't accept this improvement, though. For the purpose of documentation, did your tests yield migrate messages explaining which failed?

@rosiel rosiel merged commit 415d8de into Islandora:2.x Mar 2, 2022
@adam-vessey adam-vessey deleted the feature/edtf-constraints branch February 9, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants