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

chore: Convert some parts of Date validator to php8 syntax and add some tests to check userland retrocompatibility. #177

Open
wants to merge 1 commit into
base: 2.48.x
Choose a base branch
from

Conversation

codisart
Copy link
Contributor

Signed-off-by: codisart [email protected]

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

It seems to me that the DateValidator would profit of the php8 syntax.
I have also added some tests to allow a check on userland breaking changes.

@codisart codisart force-pushed the chore-date-validator-php8-syntax-non-BC branch from 01899b2 to cba497b Compare February 21, 2024 20:35
@codisart codisart changed the base branch from 2.31.x to 2.48.x February 21, 2024 20:37
@codisart codisart force-pushed the chore-date-validator-php8-syntax-non-BC branch 2 times, most recently from 533d4f0 to d77561a Compare February 21, 2024 21:17
…me tests to check userland retrocompatibility.

Signed-off-by: codisart <[email protected]>
@codisart codisart force-pushed the chore-date-validator-php8-syntax-non-BC branch from d77561a to 78d2570 Compare February 21, 2024 22:09
Copy link
Member

@Ocramius Ocramius 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:

src/Date.php Show resolved Hide resolved
src/Date.php Show resolved Hide resolved
test/TestAsset/CustomDate.php Show resolved Hide resolved
use DateTimeInterface;
use Laminas\Validator\Date;

final class CustomDate extends Date
Copy link
Member

Choose a reason for hiding this comment

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

Let's document why we introduced this specific asset

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants