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.

Sign up for GitHub

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

Jump to bottom

[Validator] Add support for types (ALL*, LOCAL_*, UNIVERSAL_*, UNICAST_*, MULTICAST_*, BROADCAST) in MacAddress constraint #54473

Merged
merged 1 commit into from
Apr 13, 2024
Merged

[Validator] Add support for types (ALL*, LOCAL_*, UNIVERSAL_*, UNICAST_*, MULTICAST_*, BROADCAST) in MacAddress constraint #54473

merged 1 commit into from
Apr 13, 2024

Conversation

Ninos
Copy link
Contributor

@Ninos Ninos commented Apr 3, 2024 •
edited
Loading

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Before some months we added the MacAddress contraint to v7.1, see also #51862

This MR also adds support for validating unicast/multicast, local/universal or any (default) mac address versions. For more informations, see:
https://en.wikipedia.org/wiki/MAC_address#Ranges_of_group_and_locally_administered_addresses

PS: May we should rename PRIVATE & PUBLIC to LOCAL & UNIVERSAL to be a bit more consistent with naming in mac address standard. Also then maybe version attribute to type. .. Just let me know (already prepared changes locally) :-)

@carsonbot carsonbot added this to the 7.1 milestone Apr 3, 2024
@stof
Copy link
Member

stof commented Apr 3, 2024

Naming that setting version is indeed weird as it is not a version at all. And using the actual names of the concept rather than PUBLIC and PRIVATE makes sense.

@Ninos
Copy link
Contributor Author

Ninos commented Apr 3, 2024 •
edited
Loading

Naming that setting version is indeed weird as it is not a version at all. And using the actual names of the concept rather than PUBLIC and PRIVATE makes sense.

@stof thank you! I changed the code & naming a bit, think it's a bit cleaner now. Thought about splitting the different types (unicast/multicast & local/universal) into different attributes (don't ask me about possibilities for attribute naming...), but this would increase the complexity a bit, if someone somehow needs to validate e.g. broadcast (ff:ff:ff:ff:ff:ff). Currently broadcast is LOCAL_MULTICAST, so not fully wrong.
I just didn't add BROADCAST as additional version, because this needs a bit more coding (we need to format the mac before validation & then check e.g. for ffffffffffff or bits/int), haven't found a more elegant/short solution. And normally if someone needs to validate against broadcast, this could be done with LOCAL_MULTICAST + may additional Contraint (EqualTo?).

PS: May we could also rename version to e.g. type, but for me with current naming it's ok now. Just let me know :-)

@Ninos Ninos changed the title [Validator] Add support for versions (PUBLIC, PRIVATE, ALL) in MacAddress constraint [Validator] Add support for versions (ALL, LOCAL_*, UNIVERSAL_*, UNICAST_*, MULTICAST_*) in MacAddress constraint Apr 3, 2024
@Ninos Ninos force-pushed the constraint-mac-versions branch 3 times, most recently from 589cd04 to 300cf1c Compare April 3, 2024 17:16
@Ninos
Copy link
Contributor Author

Ninos commented Apr 3, 2024

I added broadcast validation & also renamed version to type (may in some hundreds years there will be a new mac address version... :D)

@Ninos Ninos changed the title [Validator] Add support for versions (ALL, LOCAL_*, UNIVERSAL_*, UNICAST_*, MULTICAST_*) in MacAddress constraint [Validator] Add support for types (ALL*, LOCAL_*, UNIVERSAL_*, UNICAST_*, MULTICAST_*, BROADCAST) in MacAddress constraint Apr 3, 2024
@Ninos Ninos force-pushed the constraint-mac-versions branch 3 times, most recently from 93d6f02 to 02d3f21 Compare April 3, 2024 17:51
@Ninos Ninos force-pushed the constraint-mac-versions branch 3 times, most recently from 638ab7f to cc07a5a Compare April 11, 2024 19:40
@Ninos Ninos mentioned this pull request Apr 12, 2024
Merged
fabpot
fabpot requested changes Apr 13, 2024
View reviewed changes
src/Symfony/Component/Validator/Constraints/MacAddressValidator.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/MacAddressValidator.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/MacAddressValidator.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Constraints/MacAddress.php
public function __construct(
public string $message = 'This value is not a valid MAC address.',
public string $type = self::ALL,
Copy link
Member

Choose a reason for hiding this comment

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

Not a BC break as the constraint was added in 7.1.

@Ninos @fabpot
[Validator] Add support for types (ALL*, LOCAL_*, UNIVERSAL_*, …
16b9210 
…`UNICAST_*`, `MULTICAST_*`, `BROADCAST`) in `MacAddress` constraint
@fabpot
Copy link
Member

fabpot commented Apr 13, 2024

Thank you @Ninos.

@fabpot fabpot merged commit e4c7068 into symfony:7.1 Apr 13, 2024
6 of 10 checks passed
@Ninos Ninos deleted the constraint-mac-versions branch April 13, 2024 16:33
@fabpot fabpot mentioned this pull request Apr 13, 2024
Closed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 15, 2024
@javiereguiluz
minor #19784 [Validator] Add support for types (ALL*, LOCAL_*, `U…
28abe15 
…NIVERSAL_*`, `UNICAST_*`, `MULTICAST_*`, `BROADCAST`) in `MacAddress` constraint (Ninos)

This PR was merged into the 7.1 branch.

Discussion
----------

[Validator] Add support for types (`ALL*`, `LOCAL_*`, `UNIVERSAL_*`, `UNICAST_*`, `MULTICAST_*`, `BROADCAST`) in `MacAddress` constraint

See also following pull request:
symfony/symfony#54473

Commits
-------

0ed185a [Validator] Add support for types (`ALL*`, `LOCAL_*`, `UNIVERSAL_*`, `UNICAST_*`, `MULTICAST_*`, `BROADCAST`) in `MacAddress` constraint
@fabpot fabpot mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof requested changes

@xabbuh xabbuh xabbuh left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Ninos @stof @fabpot @nicolas-grekas @xabbuh @carsonbot