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

better support for item creation#28

Merged
flack merged 11 commits into
openpsa:masterfrom
symfony-cmf:cmf
Dec 11, 2012
Merged

better support for item creation#28
flack merged 11 commits into
openpsa:masterfrom
symfony-cmf:cmf

Conversation

@dbu
Copy link
Copy Markdown
Collaborator

@dbu dbu commented Oct 27, 2012

@adou600 did refactorings to support object creation.

i open this PR and will start reviewing. more feedback welcome!

basically this adds:

  • use EntityInterface rather than plain object in mapper store() method so metadata is available
  • rdf type factory can also provide metadata for a rdf type, not only for php class
  • rename collection type attribute from "controller" to "type" in xml mapping metadata

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@flack: wdyt? the thing is that we should never use short names. maybe we should make the type name expanded in the first place and shorten it in rendering?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I haven't really checked yet, but was this function moved here or copied? Having the same funtion twice in the lib is of course very ugly, so a separate helper class would be a way out of this.

Originally, I had a JsonLd class in the repo that wrapped this functionality, but at some point, this became impractical. The big headache for any solution is imo to keep track of all possible namespaces. I would say that this screams for a global registry, but unfortunately, global state was declared evil at some point...

Expanding the names during type construction sounds like a good idea, though. If we can make sure that all rendering functions have access to the namespace map for their type, this might work. And I think it could also help us to avoid rendering unused namespaces, if we manage to track which namespaces have been "reverse resolved" during a given render call.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

maybe we even should push expanding namespaces out to the rdf drivers (so the xml driver would expand them while parsing xml, and the array parser would process the passed-in array).

in #29 i propose the topic of improving the rendering, i think this is related. but for now i think the approach would be to expand everything and then make sure the entity has its namespace mapping so it can declare them and map back url to prefix back when rendering.

@dbu
Copy link
Copy Markdown
Collaborator Author

dbu commented Oct 27, 2012

@adou600 with these changes, a mapping as in <collection rel="skos:related" identifier="tags" tag-name="ul" /> seems to become invalid, which is bad. in this case the tags are not entities and not created over ajax but directly added to the parent entity. we need to be less strict.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not really familiar with the components used here, but I find it a bit confusing that for a class that has to do with doctrinePhpCr, an exception class from symfony is imported. Especially since in the code below, two time PHP's vanilla \RunTimeException is used, and then one time the RuntimException from symfony.

Like I said, I don't really know how this all fits together, but I was always under the impression that doctrine and symfony were two sepearate entities, i.e. that you could use doctrine (and therefore potentially this class) without symfony. If that's the case, then creating an interdependency with the exception class seems unnecessary

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

totally right, this runtimeexception use statement should read use \RuntimeException, we create a needless dependency on a symfony component otherwise. and additionally it would be wrong to throw this. if any we could use this idea of extending a base exception for createphp to allow catching it specifically. don't think we need to, but if then it should be a separate PR.

@dbu
Copy link
Copy Markdown
Collaborator Author

dbu commented Oct 27, 2012

oh wow, i think i will write a proposal what exactly we could do regarding the whole type/rev story and then do a PR to just fix that. this becomes too big.

@dbu
Copy link
Copy Markdown
Collaborator Author

dbu commented Oct 29, 2012

we should wait with this PR until #34 is done, then merge master into here and solve the undoubtly occuring merge conflicts and finalize object creation here.

@lsmith77
Copy link
Copy Markdown

#34 seems to be merged.

@flack
Copy link
Copy Markdown
Collaborator

flack commented Nov 17, 2012

indeed. But right now, this PR cannot be automatically merged, I suppose it needs syncing with the latest changes

@dbu
Copy link
Copy Markdown
Collaborator Author

dbu commented Nov 18, 2012

indeed, it needs sync as i changed quite fundamental things about item creation to make it possible to do this PR in a clean way.

but first we need to sort out the problems we have with non-entity collections and see if that has any impact: #37

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.

4 participants