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

Allow laminas/laminas-validator 3.0 #83

Closed
wants to merge 0 commits into from

Conversation

marcelthole
Copy link
Contributor

@marcelthole marcelthole commented Jul 17, 2024

Q A
BC Break no

Description

This is the preparation PR for the release of https://github.com/laminas/laminas-validator/tree/3.0.x

As far as i see there aren't any BC breaks here in this package because there is only this usage of the Hostname Validator:

            // set up a validator that check if the hostname is legal (not spoofed)
            $hostnameValidator = new HostnameValidator([
                'allow'       => HostnameValidator::ALLOW_ALL,
                'useIdnCheck' => false,
                'useTldCheck' => false,
            ]);
            // If invalid. Reset the host & port
            if (! $hostnameValidator->isValid($host)) {
                $host = null;
                $port = null;
            }

All of these options and methods are still available in the unreleased 3.0.x branch

The important change would be this signature change

public function isValid(mixed $value): bool

https://github.com/laminas/laminas-validator/blob/3.0.x/src/Hostname.php#L1814

But in the current implementation of laminas-http the $host variable is string and i think this should be fine

@gsteel
Copy link
Member

gsteel commented Jul 17, 2024

Thanks @marcelthole - This is a tad premature though considering 3.0 is not out yet. Let's hang on so that CI can test with both 2.x and 3.x 👍

@marcelthole
Copy link
Contributor Author

yeah no need to merge it yet. Was only the overall question if this is fine to allow 2.x and 3.x
i will check some other packages and also prepare a pull requests for these packages

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.

2 participants