Now that we have serializer support for JSON and XML. The last one it makes sense to implement is YAML. Then we have the most common serialization formats out of the box. Plus, as we already have the YAML component, this is a relatively small amount of code.
This could open out some (possibly crazy) possibilities using rest web services with configuration too.
Here is an initial patch, we have to create the encoder from scratch as symfony doesn't have one, but this could be patched upstream later if possible?
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff-1897612-66-mr-3399.txt | 2.71 KB | Bhanu951 |
#70 | 1897612-nr-bot.txt | 4.53 KB | needs-review-queue-bot |
#66 | interdiff_64-66.txt | 826 bytes | ravi.shankar |
#66 | 1897612-66.patch | 8.41 KB | ravi.shankar |
#64 | interdiff-1897612-63-64.txt | 599 bytes | yogeshmpawar |
Issue fork drupal-1897612
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedAdded some quick tests too. Not sure if we want to actually test against a yaml string?
Comment #2
damiankloip CreditAttribution: damiankloip commentedSorry, this patch.
Comment #3
dawehnerThis is really looking great so far.
PS: Do we still need the needs tests tag?
Missing starting "\"
Maybe we should explain why application/x-yaml or text/yaml is used, I guess because of http://stackoverflow.com/questions/332129/yaml-mime-type
Hey that comment is stolen :)
Comment #4
damiankloip CreditAttribution: damiankloip commentedI'm not sure we need the extend the serializerAwareEncoder.
Comment #5
damiankloip CreditAttribution: damiankloip commentedSorry, without the use for that too. Also just realised I x-posted the last patch. So thanks! I've addressed your comments too, except the mime type one: Any good suggestions? Also, do you think we should add all the possibilities? seems a bit overkill. I can if you want though.
Comment #6
dawehnerOh to be honest I didn't wanted it to be dropped, as that's helpful for everyone reading the code.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedJulian Reschke is the person we should be listening to in that issue, even though he doesn't get the upvotes.
I found a message he sent shortly after posting that, indicating that he wanted to get one formally registered. I'm not sure where that went.
Comment #8
damiankloip CreditAttribution: damiankloip commentedRerolled after #1850704: Available serialization formats.
So are we saying we want to add 'application/vnd.yamlorg.yaml' to the list of mime types?
Comment #10
damiankloip CreditAttribution: damiankloip commented#8: 1897612-9.patch queued for re-testing.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedApparently, MediaWiki does use application/yaml. That might be our best bet, even though it doesn't follow JR's advice.
Do we know which consumers are going to be requesting it?
Comment #12
damiankloip CreditAttribution: damiankloip commentedSo we go with this for now?
I think this is something we can easily modify a bit later if we find out we need to, that's well within reason.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedThe suggestions are pretty minor, so I don't feel strongly about them and would be ok with the patch going ahead. I realize some of the code style errors made it in to JSON-LD module, so those errors may just be from referring to the existing code.
Should this be called SerializerFormatSubscriber? In JSON-LD, it's just called JsonldSubscriber (I think klausi implemented that, but I agree with his naming). I would just call it SerializationSubscriber
Small code style issue, the class should have a slash at the front.
Why do we have a priority here?
Do we feel confident that the $normalized array will be properly handled by the dumper? As confident as we are that json_encode() will always give us the results we expect?
If we aren't confident, then we should actually test the output like we do with XML. I don't have enough background on how we've been using the YAML dumper to know.
Comment #14
damiankloip CreditAttribution: damiankloip commentedThank s for the feedback. I will reroll with these changes.
Comment #15
damiankloip CreditAttribution: damiankloip commentedI think I would be pretty confident with the YAML dumper. We use the same implementation in core, and this is obviously used alot :). So I think comparing to this is for the expected output is pretty safe.
I have made those other changes.
Comment #16
Crell CreditAttribution: Crell commentedIs there a use case for this in core?
The REST and CMI teams already concluded that since CMI files are best deployed via Git, not via REST, so we're not supporting ConfigEntities. That means this wouldn't be useful for doing configuration deployments.
If there's a core-friendly use case here we can move forward. If not, I think this would be better off in contrib where we can sort out the mime type issue and other questions at our leisure.
Comment #17
damiankloip CreditAttribution: damiankloip commentedWell, technically there probably isn't use cases for json, xml, or json-ld in core either. It seems like a good idea to have support for the most commonly used formats out of the box, and as we already have the yaml component, it's a small patch.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedThere are two use cases for JSON-LD in core: content deployment and Spark. We've determined the particular advantages that JSON-LD has in terms of those two use cases.
I think Crell has a point in pushing for a core use case. There is a significant disadvantage to having code live in core... it's much harder to change it than it is if it lives in a contrib module. Since there are still open questions around this (who are the consumers and what media type will they handle), I think it makes more sense for it to live in contrib, where the users can influence the implementation more.
Comment #19
damiankloip CreditAttribution: damiankloip commentedWhat about json and xml? It's not about pushing config around with yaml, we could do that with json if we wanted to. Having this format too will mean we have all the common formats. That can't be too much of a bad thing.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedJSON is used for AJAX, which is a pretty clear core use case. TBH, I would personally be fine with moving XML out of core too.
Comment #21
scor CreditAttribution: scor commentedwell, maybe the question to consider is what are the overheads of having many serialization formats in core. YAML has an unstable MIME type, ok. JSON and XML are much more mature. Leaving aside the MIME type discussion, what is the code overhead everytime we add/allow a format? How much code do we need to add/maintain everytime we add a format? I'm assuming we're relying on the upstream serializers like PHP or Symfony, which are available no matter what. We just have to decide whether we allow them or not. What about performance overheads in adding more formats?
Edit: I don't particular care for YAML or XML one way or another, just trying to get the bigger picture here.
Comment #22
Crell CreditAttribution: Crell commentedTo be clear, I am not against being able to serialize to YAML. I'm just looking at it and going "uh, why?", especially since, as Lin notes, core moves more slowly than contrib. Aside from "ticking a box", does YAML serialization being in core buy us anything? If so, then I'm fine with including it.
We should have at minimum one non-JSON format in core to validate that serialization is in fact pluggable; XML already fills that role, though, plus gives a foundation for doing other XML formats. (Atom, SVG, etc. can probably reuse at least some code.)
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedThis only matters for denormalizing, but are we certain that we wouldn't open up security holes with YAML? See the "What went wrong" paragraph of What The Rails Security Issue Means For Your Startup.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedActually, now that I think of it, I wonder if it might matter for serializing as well. Could an attacker construct an entity body such that it triggered a YAML vulnerability when serializing? I legitimately have no idea.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedWe've been discussing this in IRC, but I just want to post it here for history's sake.
I would not feel comfortable with this being added unless we got full review and sign off from the security team.
I personally do not think it's worth going to the effort, but am fine with it if someone else does want to pursue that.
Comment #26
scor CreditAttribution: scor commentednot speaking on behalf of the team, but just my personal feeling as a member: it seems unlikely the security team would have the time or resources to do a full review on this, especially since the benefits of adding YAML in this instance (serializer) are still unclear/unknown. contrib seems like a good place for such serializer to live for now.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedSo here's what chx said about the exploitability:
That's what this functionality would do. As such, I think we should make it a wontfix for core. chx and others are talking about ways to improve Symfony's security (#1907170: Very simple config files can't be read), if those move forward this could happen in contrib.
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedSymfony actually has this capability since mid-January. So this is just postponed on us updating the Yaml component.
http://symfony.com/blog/security-release-symfony-2-0-22-and-2-1-7-released
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedIs there anyone actually asking for this functionality? Moshe, Crell, and myself have all asked what the use case is or who the consumers would be, and scor points out that the benefits are unknown. We still haven't established these basic benefits, which I think should be a blocker in and of itself... nothing should get in core (and thus have to be maintained) unless it has a clear benefit. Contrib is for hypotheticals.
I do realize that Symfony added new, safer versions of Yaml:parse(). However, other projects have been seriously burned by YAML security holes, which were clearly not obvious to them for years... what makes us think that a few weeks worth of security review has fixed all the holes.
Why play with fire in core when we don't have a use case in mind?
Comment #29.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary.
Comment #30
yched CreditAttribution: yched commentedReminder: The "Serialization API" handbook page at https://drupal.org/node/1900312 still has an example about "adding YAML serializer", with a "TODO Insert example once YAML patch [this issue here] is finalized" :-)
Comment #35
Chi CreditAttribution: Chi commentedThere is pretty solid use case for having Yaml serializer - deploying content.
Default content module uses Json format which is not as readable as Yaml. Demo content and Yaml content modules have own implementation of Yaml import but not export. I suppose Yaml serializer would help a lot on this.
Comment #36
AndyF CreditAttribution: AndyF at TES Global commentedFYI I've made a module from the patch (many thanks @damiankloip!). It's just a sandbox at the mo. I'll promote it in a few weeks if there's no further discussion - just in case this or Chi's message sparks more dialogue and people end up wanting it in core again :p
https://www.drupal.org/sandbox/andy/2924284
Thanks
Comment #39
andypostComment #40
manojapare CreditAttribution: manojapare as a volunteer commentedTo make sure Drupal\Core\Serialization onKernelRequest() event callabck is called just before Symfony\Component\HttpKernel\EventListener\RouterListene onKernelRequest(). Increased the priority for onKernelRequest() event callback from 0 to 40.
Unless yaml mime type is not taken for REST endpoints.
Comment #41
andypost@manojapare thank you for reroll, please add #40 as code comment to keep knowledge of why 40
It needs some codestyle changes as well
should be removed
Needs a code comment about why
should be short array syntax
Comment #42
AndyF CreditAttribution: AndyF commentedIt looks like perhaps we should be taking a different approach since #2445723: Use the $request format instead of the ContentNegotation. landed of altering the DIC at build time to register the format with the middleware, similar to how
\Drupal\hal\HalServiceProvider
does it?Comment #43
xjmComment #44
manojapare CreditAttribution: manojapare as a volunteer commentedRerolled the patch to 9.1.x-dev and taken care suggestions by @andypost.
Comment #46
manojapare CreditAttribution: manojapare as a volunteer commentedSorry my bad, fixed codesniffer list.
Comment #47
johnwebdev CreditAttribution: johnwebdev commentedWhy do we need to extend the same encoder we are wrapping?
It does more than just register formats currently.
s/Event/event
Copy error.
s/Event/event
This message is not really that helpful. Also missing test coverage.
Here we can use {@inheritdoc}
s/yaml/YAML
s/callabck/callback
This line exceeds 80 characters.
Comment #48
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedComment #49
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedHi here is the updated testcase for patch 45 uploaded in #46
Comment #50
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedHi @johnwebdev
I'd not get much about #1 and #6.
The attached patch has the suggested improvements and the interdiff of #49 and #50.
Comment #51
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedThis patch contains the fixes for the 2 failures in #49 and #50
Comment #52
shaktikComment #53
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedComment #54
shaktikComment #55
Rkumar CreditAttribution: Rkumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUnassigning this.
Comment #58
bradjones1Adjusting title since this is a bit confusing when searching for other issues relating to yaml serialization, e.g. for parsing config. This is more for request/response serialization akin to JSON and XML.
Comment #60
andypostComment #61
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled #53
Comment #62
shaktikComment #63
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedTried to fix the custom error. Please have a look.
Comment #64
yogeshmpawarResolved Custom Commands errors & also added an interdiff.
Comment #66
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed tests of patch #64.
Comment #67
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #70
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #71
Akhil Yadav CreditAttribution: Akhil Yadav commentedAdded patch against #66 in 10.1 version
Comment #72
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedSetting as need review for tests to run.
Comment #74
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedSeems changes made to core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php in #66 are missing in #71.
Comment #76
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedRaised MR based on #66 against branch 10.1.x
Comment #77
apaderno@Akhil The previous patch is 8.41 KB. How can a re-roll be 3.97 KB?
Comment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This could use an issue summary update. Example What is the proposed solution exactly?