Skip to content

Conversation

norkunas
Copy link
Contributor

@norkunas norkunas commented Apr 2, 2021

Q A
Branch? main
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

Add a PropertySchemaUrlRestriction to transform Url validation constraint into json schema.

@norkunas norkunas force-pushed the property-schema-url-restriction branch from fe91074 to 27825b7 Compare April 2, 2021 07:13
@guilliamxavier
Copy link
Contributor

Please remove me from the description 😆 I'm already checking https://github.com/api-platform/core/pulse from time to time these days (also I'm not part of the team)

If it may help: the test failure for "lowest" is related to symfony/validator@adab212 (introduced in v4.4.8)

But string manipulation and generating a huge pattern in the schema... is it really worth it? 🤔 Wouldn't it be enough to add something like this in the existing PropertySchemaFormat?

         if ($constraint instanceof Email) {
             return ['format' => 'email'];
         }
+
+        if ($constraint instanceof Url) {
+            return ['format' => 'uri'];
+        }
 
         if ($constraint instanceof Uuid) {
             return ['format' => 'uuid'];
         }

This would not be strictly equivalent (also ambiguity between "URL", "URI" and "URI reference"), but that's already true for Email (3 different validation modes)... Also Url is probably less used...

Anyway let's wait for a proper review from a team member 😄

@norkunas
Copy link
Contributor Author

norkunas commented Apr 2, 2021

I just cc'ed because you were interested in these PR's :D

@norkunas
Copy link
Contributor Author

norkunas commented Apr 2, 2021

IMHO if backend validates based on some pattern then it should be exposed nevertheless..

@norkunas norkunas force-pushed the property-schema-url-restriction branch 2 times, most recently from ebb332e to 02a6efa Compare April 2, 2021 08:32
@norkunas
Copy link
Contributor Author

norkunas commented Apr 2, 2021

Included also Hostname constraint as it's uri related

@alanpoulain
Copy link
Member

I agree with @guilliamxavier. Since the format is used, I don't think it's a good idea to have the regexp as well.
Since URI is a specification, there should be no difference between the "client" regular expression and the backend one.

@norkunas
Copy link
Contributor Author

norkunas commented Apr 2, 2021

But how would you distinguish then allowing only some other specific protocols?

@alanpoulain
Copy link
Member

You would not be able to, that's right. Personally, I think it's a little bit too much detailed though. And the implementation is very dependent of the pattern in UrlValidator. If something change, it will break.
But maybe I'm wrong on this one. @soyuka, do you have an opinion on this matter?

@norkunas norkunas force-pushed the property-schema-url-restriction branch from 02a6efa to 925332a Compare April 6, 2021 08:48
@norkunas norkunas force-pushed the property-schema-url-restriction branch from 925332a to 942013b Compare April 7, 2021 04:26
@norkunas norkunas changed the title Add support for generating property schema with Url restriction Add support for generating property schema format for Url and Hostname Apr 7, 2021
@norkunas norkunas force-pushed the property-schema-url-restriction branch from 942013b to cca1ba5 Compare April 7, 2021 04:29
@norkunas norkunas force-pushed the property-schema-url-restriction branch from cca1ba5 to 989df9a Compare April 7, 2021 04:56
@norkunas
Copy link
Contributor Author

norkunas commented Apr 7, 2021

Removed the pattern part.

@soyuka soyuka merged commit c7e18e3 into api-platform:main Apr 7, 2021
@soyuka
Copy link
Member

soyuka commented Apr 7, 2021

Tyvm @norkunas !

@norkunas norkunas deleted the property-schema-url-restriction branch April 7, 2021 12:30
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.

4 participants