Skip to content

Conversation

@markstory
Copy link
Member

@markstory markstory added this to the 4.next milestone Apr 5, 2020
@markstory markstory requested a review from saeideng April 5, 2020 04:04
@markstory markstory merged commit 822ddcf into 4.next Apr 7, 2020
@markstory markstory deleted the after-marshall-event branch April 7, 2020 13:39
) {
// Don't accept people who have a name starting with J on the 20th
// of each month.
if (substr($entity->name, 1) == 'J' && date('d') === 20) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

date('d') is string .

Use either idate('d') or compare to '20'

Copy link
Member

Choose a reason for hiding this comment

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

(int) cast would also suffice, I fix that up.

@saeideng
Copy link
Member

saeideng commented Apr 7, 2020

LGTM
only I think better to mention that $data and $options are unchangeable , here or in doc block
and better to show example between 2 field that on validation process it is impossible

if ($entity->name, 1) == 'J' && $entity->xyz === '123')
         $entity->setError('name', '......');

//or 
if ($entity->x === 1)
     $entity->setError('y', '....message.....');

though current example and details are fine 👍

@dereuromark
Copy link
Member

I am not sure if we should really keep the $data
The main interest here is the entity itself and its fields. And that you can issue error messages and alike.
The data I do not see much use for, and it makes this callback different than all others (having 1 more arg).

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.

5 participants