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

Fix validate check #77

Merged
merged 4 commits into from
Jan 25, 2022
Merged

Conversation

nigelgbanks
Copy link
Member

Fixes validation in two small cases
Empty strings are now always prohibited and checking
for intervals does not error if a non-interval
value is given.

Previously:

use Drupal\controlled_access_terms\EDTFUtils;
EDTFUtils::validate("", TRUE, TRUE, TRUE);
// yields [];
EDTFUtils::validate("", FALSE, TRUE, TRUE);
// yields [ "Could not parse the date ''." ]
EDTFUtils::validate("1985-04-12T23:20:30Z", TRUE, TRUE, TRUE);
// yields [ "Date intervals cannot include times." ]

Now:

use Drupal\controlled_access_terms\EDTFUtils;
EDTFUtils::validate("", TRUE, TRUE, TRUE);
// yields [ "Cannot parse empty value." ];
EDTFUtils::validate("", FALSE, TRUE, TRUE);
// yields [ "Cannot parse empty value." ];
EDTFUtils::validate("1985-04-12T23:20:30Z", TRUE, TRUE, TRUE);
// yields [ ]

Empty strings are now always prohibited and checking
for intervals does not error if a non-interval
value is given.

Previously:

```php
use Drupal\controlled_access_terms\EDTFUtils;
EDTFUtils::validate("", TRUE, TRUE, TRUE);
// yields [];
EDTFUtils::validate("", FALSE, TRUE, TRUE);
// yields [ "Could not parse the date ''." ]
EDTFUtils::validate("1985-04-12T23:20:30Z", TRUE, TRUE, TRUE);
// yields [ "Date intervals cannot include times." ]
```

Now:

```php
use Drupal\controlled_access_terms\EDTFUtils;
EDTFUtils::validate("", TRUE, TRUE, TRUE);
// yields [ "Cannot parse empty value." ];
EDTFUtils::validate("", FALSE, TRUE, TRUE);
// yields [ "Cannot parse empty value." ];
EDTFUtils::validate("1985-04-12T23:20:30Z", TRUE, TRUE, TRUE);
// yields [ ]
```
@seth-shaw-unlv
Copy link
Contributor

Happy to review, although it probably won't be until tomorrow. In the mean-time we need to address those failing builds. I know we recently saw this on other repos, but I don't recall what changes we made to the github actions config to fix it.

@whikloj
Copy link
Member

whikloj commented Jan 20, 2022

You need to uninstall mysql-client and mysql-common before trying to install it.
https://github.com/Islandora/islandora/pull/861/files#diff-453d9c758046a0eacc2d4f41b1f72b223fe486676bc31b09dbddc94188ce434dR177

Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Will merge after the github workflow is updated for mysql and we get green on the build.

@seth-shaw-unlv
Copy link
Contributor

@nigelgbanks, my last day at work before paternity leave is this Friday. Could you make the build changes before then so I can merge this before I check-out for a few weeks?

@nigelgbanks
Copy link
Member Author

Tried to apply the changes that @whikloj suggested and it got past the MySQL installation, but now just hangs on the Setup Drupal step. I can't restart the job or stop it since I'm not an admin of the repository. From what I've read it may take 6 hours to stop on it's own (https://github.saobby.my.eu.orgmunity/t/step-hangs-indefinitely/17720).

@nigelgbanks
Copy link
Member Author

nigelgbanks commented Jan 25, 2022

@seth-shaw-unlv & @whikloj for this to get fixed this other pull must be first merged:

Islandora/islandora_ci#8

I've tested it on a my own fork (which is passing):
https://github.com/nigelgbanks/controlled_access_terms/runs/4934638675?check_suite_focus=true

After that has been merged you'll have to manually rerun the Github Actions in the pull request.

@seth-shaw-unlv seth-shaw-unlv merged commit 3835e2f into Islandora:2.x Jan 25, 2022
@nigelgbanks nigelgbanks deleted the fix-validate-check branch January 25, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants