Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

Added escape characters for repository class name #66

Merged
merged 5 commits into from
Mar 19, 2014

Conversation

rudott
Copy link
Contributor

@rudott rudott commented Feb 28, 2014

Fixed issue: #65

This will change the createphp_attributes output to something like:
xmlns:schema="http://schema.org/" typeof="schema:Page" about="Acme-DemoBundle-Entity-Page|id=1"

And added the unescape for getBySubject class name.

Fixed issue: openpsa#65

This will change the createphp_attributes output to something like:
xmlns:schema="http://schema.org/" typeof="schema:Page" about="Acme-DemoBundle-Entity-Page|id=1"

And added the unescape for getBySubject class name.
@@ -70,7 +71,7 @@ public function getBySubject($subject)
if (count($ids) < 2) {
throw new RuntimeException("Invalid subject: $subject");
}
$class = ltrim($ids[0], '/'); // if we get the / from the url, this breaks the class loader in a funny way.
$class = $this->unescape(ltrim($ids[0], '-')); // if we get the - from the url, this breaks the class loader in a funny way.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you might still get / at the beginning of the url. it has nothing to do with namespaces, but with how the url pattern is built.

@@ -31,6 +31,7 @@ class DoctrineOrmMapper extends BaseDoctrineRdfMapper
'<' => '%3C',
'>' => '%3E',
'|' => '%7C',
'\\' => '-',
Copy link
Collaborator

Choose a reason for hiding this comment

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

the consistent would probably be to use %92 rather than -.

the other option is to not use the escape function at all for the class name, but just string-replace \ with -

@rudott
Copy link
Contributor Author

rudott commented Feb 28, 2014

@dbu thanks for your feedback, I created a new commit with all the requested changes.

@@ -70,7 +71,7 @@ public function getBySubject($subject)
if (count($ids) < 2) {
throw new RuntimeException("Invalid subject: $subject");
}
$class = ltrim($ids[0], '/'); // if we get the / from the url, this breaks the class loader in a funny way.
$class = str_replace('-', '\\', ltrim($ids[0], '/')); // if we get the / from the url, this breaks the class loader in a funny way.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we separate this in two lines?

$class = ltrim($ids[0], '/'); // if we get the / from the url, this breaks the class loader in a funny way.
$class = str_replace('-', '\\', $class);

@dbu
Copy link
Collaborator

dbu commented Feb 28, 2014

@flack i think it makes sense to do this. not exactly sure why it worked for me but fails for @rudott but seems a sane change to me.

@rudott
Copy link
Contributor Author

rudott commented Feb 28, 2014

@dbu thanks for your quick feedback!
Superb 👍

rudott added a commit to rudott/symfony-application that referenced this pull request Feb 28, 2014
@flack
Copy link
Collaborator

flack commented Mar 12, 2014

Hi guys, sorry for the late reply, I'm on vacation until next week and only have sporadic connectivity. I'll look into this & will comment or merge when I have time

@rudott
Copy link
Contributor Author

rudott commented Mar 14, 2014

@flack okay no problem, let me know when you can look in to it.

I already made two implementations with CreateJS and Doctrine ORM and it works like a charm :-)

https://github.com/rudott/create.js-kunstmaan-integration (based on the Kunstmaan bundles)
https://github.com/rudott/symfony-application/tree/createjs (based on a Symfony application from @endroid)

@flack
Copy link
Collaborator

flack commented Mar 19, 2014

@rudott Looks good so far!If you have the time, it would be nice to have a unit test which exhibits the problem. If not, just let me know, then I'll merge as-is

This wil validate if the Subjectname does not contain backslashes
@rudott
Copy link
Contributor Author

rudott commented Mar 19, 2014

@flack I added a test for the subjectname in the DoctrineOrmMapperTest.

flack added a commit that referenced this pull request Mar 19, 2014
Added escape characters for repository class name
@flack flack merged commit 8627540 into openpsa:master Mar 19, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants