Fix URI generic syntax delimit path components by slash ("`/`") #105

hakre · 2014-06-09T15:06:55Z

Parts of justinrainbow/json-schema are using the backslash ("\") in
error on occasion.

hakre · 2014-06-09T15:07:56Z

This first commit at least fixes the issue for me on a windows test-system.

The issue also highlighted wrong usage of DIRECTORY_SEPARATOR in other places. It also made visible a certain amount of duplicate code between UriRetriever and UriResolver.

hakre · 2014-06-09T15:17:26Z

For reference: URI uses the slash ("/"). See 1.2.3. Hierarchical Identifiers (Uniform Resource Identifier (URI): Generic Syntax; RFC 3986).

If you follow any of these standards, there are existing components that do resolvement of URI parts, for example, you should be able to use Net_URL2 which is pretty robust.

hakre · 2014-06-09T15:35:24Z

Two little refactoring commits on the house to align and remove some duplicate code.

hakre · 2014-07-24T15:41:08Z

@justinrainbow same here.

bighappyface · 2015-03-25T03:50:56Z

@hakre would you please rebase the latest?

hakre · 2015-03-26T20:58:39Z

@bighappyface: Rebased + refreshed and a little squashed.

bighappyface · 2015-03-26T21:06:15Z

My bad, it was all removed in 43f7a94

bighappyface · 2015-03-26T21:09:06Z

I think this is a solid refactor and dedup. Thank you.

+1

I will merge this with another +1 from the community.

hakre · 2015-03-26T21:11:21Z

@bighappyface: The curlies have been squared now. Merge like you see fit, but don't let it rot :)

bighappyface · 2015-03-26T21:14:00Z

@hakre I am sure one of the other consumers with PRs will help review. Quid pro quo 😄

hakre · 2015-03-26T21:16:33Z

@bighappyface: Was just pushing a bit because this PR was laying around for quite some time. Just let me know if it needs refresh again or there's anything.

bighappyface · 2015-03-27T02:12:59Z

@keradus you have been helpful reviewing #134; would you mind looking over this as well?

keradus · 2015-03-27T10:09:51Z

@bighappyface sorry, don't know this repo

bighappyface · 2015-03-27T16:46:58Z

@loucho @onlinesid would you mind reviewing this PR for @hakre?

loucho · 2015-03-27T16:51:14Z

+1 👍 seems to be working and i like the corrections to the phpdocs

bighappyface · 2015-03-27T17:12:31Z

@loucho thanks for the help!

@hakre I am ready to merge this but I didn't expect such a fast turnaround for the review. I just pushed out 1.4.1 based on #132 today, and should have included this PR in it.

The only other PR I see that is close is the work being done on #134. Would you both help us determine a path forward on that work? If it looks like we will spend much more time there, I will merge this today and bump to 1.4.2. If we can make progress on that end soon, it would be nice to wait and roll multiple updates into a single new version.

Thoughts?

hakre · 2015-03-27T18:01:47Z

@bighappyface: I'd say #134 can greatly benefit from rebasing.

I've done the rebasing for the PR here as well. Should I squash the changes here together into one commit?

bighappyface · 2015-03-27T19:30:35Z

I think a single commit is best, but the rebase you have performed is great. I agree with the rebase for #134 and we can take care of that up once we determine how to tackle better validation error messaging.

keradus · 2015-03-27T19:32:44Z

IMO all PRs should contain one commit per author.

keradus · 2015-03-27T19:33:08Z

src/JsonSchema/Uri/UriResolver.php

@@ -72,7 +72,7 @@ public function generate(array $components)
     * Resolves a URI
     * 
     * @param string $uri Absolute or relative
-     * @param type $baseUri Optional base URI
+     * @param string $baseUri Optional base URI
     * @return string Absolute URI


return annotation should be separated due to PHPDoc

more context.

keradus · 2015-03-27T19:35:35Z

src/JsonSchema/Uri/UriRetriever.php

     * Guarantee the correct media type was encountered
     *
-     * @throws InvalidSchemaMediaTypeException
+     * @param UriRetrieverInterface $uriRetriever


then parameter should be typehinted

keradus · 2015-03-27T19:38:24Z

This is a fix, but no new tests were added...

bighappyface · 2015-03-27T20:19:00Z

@keradus I think we are good here. The test suite provides regression testing for this refactor:

https://github.com/justinrainbow/json-schema/blob/master/tests/JsonSchema/Tests/Uri/UriResolverTest.php

keradus · 2015-03-27T20:34:35Z

and now the docs is lying ;)

bighappyface · 2015-03-27T20:48:23Z

@keradus do you mean the various comments you made on the annotations? I am sure @hakre will adjust.

keradus · 2015-03-27T20:59:48Z

Just to show we are not good yet here ;P

Parts of justinrainbow/json-schema are using the backslash ("`\`") in error on occasion. This fixes the issue in **UriResolver** as well as in **UriRetriever**. With this commit, the *overall* usage of that DIRECTORY_SEPARATOR constant has been redacted out of this project. Additionally in a refactoring duplicate code has been removed from the classes afterwards and some minor CS fixes have been applied.

hakre · 2015-03-28T01:15:32Z

Just squashed into one commit. Also went through the comments by @keradus - thank you for taking the time to review. Technically it's correct about the test but I also think it's OK to have this lifted as this PR is mainly maintenance and low-level improvement today. The original fix isn't part of it any longer 100%. It just has over-lived itself in part.

bighappyface · 2015-03-28T17:07:19Z

@hakre thank you for the squash and response to feedback. Let's merge this in and include it in the next version bump.

Fix URI generic syntax delimit path components by slash ("`/`")

hakre · 2015-03-30T18:50:06Z

@bighappyface ACK, cleared here locally. If you have some other older rotten PR you'd like to see refreshed, let me know.

hakre force-pushed the patch-2 branch 3 times, most recently from c709553 to 43f7a94 Compare March 26, 2015 20:29

hakre force-pushed the patch-2 branch from 43f7a94 to 406eceb Compare March 26, 2015 21:09

bighappyface added the Needs Review label Mar 27, 2015

bighappyface mentioned this pull request Mar 27, 2015

Make the enum element comparison strict #132

Merged

bighappyface mentioned this pull request Mar 27, 2015

Remove warning if file doesn't exist and send correct exception #138

Closed

hakre force-pushed the patch-2 branch from 406eceb to 58642c2 Compare March 27, 2015 18:00

keradus reviewed Mar 27, 2015
View reviewed changes

hakre force-pushed the patch-2 branch from 58642c2 to 5043799 Compare March 28, 2015 01:03

hakre force-pushed the patch-2 branch from 5043799 to 0f3ca84 Compare March 28, 2015 01:14

bighappyface added a commit that referenced this pull request Mar 28, 2015

Merge pull request #105 from hakre/patch-2

34e2982

Fix URI generic syntax delimit path components by slash ("`/`")

bighappyface merged commit 34e2982 into jsonrainbow:master Mar 28, 2015

bighappyface removed the Needs Review label Mar 28, 2015

hakre deleted the patch-2 branch March 30, 2015 18:46

Fix URI generic syntax delimit path components by slash ("/") #105

Fix URI generic syntax delimit path components by slash ("/") #105

Uh oh!

Conversation

hakre commented Jun 9, 2014

Uh oh!

hakre commented Jun 9, 2014

Uh oh!

hakre commented Jun 9, 2014

Uh oh!

hakre commented Jun 9, 2014

Uh oh!

hakre commented Jul 24, 2014

Uh oh!

bighappyface commented Mar 25, 2015

Uh oh!

hakre commented Mar 26, 2015

Uh oh!

bighappyface commented Mar 26, 2015

Uh oh!

bighappyface commented Mar 26, 2015

Uh oh!

hakre commented Mar 26, 2015

Uh oh!

bighappyface commented Mar 26, 2015

Uh oh!

hakre commented Mar 26, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

keradus commented Mar 27, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

loucho commented Mar 27, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

hakre commented Mar 27, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

keradus commented Mar 27, 2015

Uh oh!

keradus Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

hakre Mar 28, 2015

Choose a reason for hiding this comment

Uh oh!

keradus Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

hakre Mar 28, 2015

Choose a reason for hiding this comment

Uh oh!

keradus commented Mar 27, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

keradus commented Mar 27, 2015

Uh oh!

bighappyface commented Mar 27, 2015

Uh oh!

keradus commented Mar 27, 2015

Uh oh!

hakre commented Mar 28, 2015

Uh oh!

bighappyface commented Mar 28, 2015

Uh oh!

hakre commented Mar 30, 2015

Uh oh!

Uh oh!

Fix URI generic syntax delimit path components by slash ("`/`") #105

Fix URI generic syntax delimit path components by slash ("`/`") #105