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

ENH allow stacked messages on FormMessage #10966

Merged

Conversation

andrewandante
Copy link
Contributor

@andrewandante andrewandante commented Oct 3, 2023

Fixes #10270

Adds a new method: FormMessage::appendMessage() which attempts to stack messages if it can, falling back to the current behaviour of setting the message if there's a mismatch.

image

src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Also FYI this has prompted me to make this issue for CMS 6

@andrewandante
Copy link
Contributor Author

Also FYI this has prompted me to make this issue for CMS 6

Hah yes that was definitely coming, I was reluctant to turn this into an API-breaking change but I think that would definitely be the best long-term approach.

@andrewandante andrewandante force-pushed the ENH/better_pwd_validation_msgs branch 5 times, most recently from 6329397 to e90de56 Compare October 4, 2023 23:19
@andrewandante
Copy link
Contributor Author

Think I've managed to get my conditions in the right order now 😅

src/Forms/Form.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
src/Forms/FormMessage.php Show resolved Hide resolved
src/Forms/FormMessage.php Show resolved Hide resolved
src/Forms/FormMessage.php Outdated Show resolved Hide resolved
tests/php/Forms/FormTest.php Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looking good. Just need to make CI happy now. There's a suggestion below about making the phpunit job happy, and the lint job I think speak for itself.

tests/php/Forms/FormTest.php Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks great and does what it says on the tin. Thanks!

@GuySartorelli GuySartorelli merged commit 87958e7 into silverstripe:5 Oct 9, 2023
15 checks passed
@GuySartorelli
Copy link
Member

This will be included in the Silverstripe CMS 5.2.0 release.

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.

Show all failed password criteria when changing a password to a not-strong-enough value
2 participants