Problem/Motivation

Serializer's interface was recently changed

All serializer interfaces (Serializer, Normalizer, Encoder) have been extended with an optional ``$context`` array. This was necessary to allow for more complex use-cases that require context information during the (de)normalization and en-/decoding steps.

As mentioned in #1880424: Handle entity references on import and code comments, the $context parameter could be useful for us.

Proposed resolution

Update Serializer using composer and change all Normalizers and Encoders to support the new interface.

CommentFileSizeAuthor
#21 1894508-21.patch61.39 KBscor
#20 1894508-20.patch60.38 KBdamiankloip
#12 1894508-11.patch52.88 KBdamiankloip
#11 1894508-11-update-serializer.patch52.09 KBlinclark
#5 1894508-03-update-serializer.patch55.04 KBlinclark
#1 1894508-01-update-serializer.patch53.37 KBlinclark
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Active » Needs review
FileSize
1894508-01-update-serializer.patch53.37 KB

This updates the Serializer code and our Normalizers. We don't have Encoders we need to worry about yet, but will if #1880424: Handle entity references on import gets in.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue tags: +WSCCI, +json-ld, +serialization

Status: Needs review » Needs work

The last submitted patch, 1894508-01-update-serializer.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented

Related: #1893470: Update dependencies, add PSR\Log

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
1894508-03-update-serializer.patch55.04 KB

Forgot about the test encoder.

Status: Needs review » Needs work
Issue tags: -WSCCI, -json-ld, -serialization

The last submitted patch, 1894508-03-update-serializer.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
Issue tags: +WSCCI, +json-ld, +serialization

#5: 1894508-03-update-serializer.patch queued for re-testing.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Talked this through in IRC. We can pin the commit for the serializer library in #1894002: Update vendor libraries and pin them to specific versions in composer.json, which is paused until Symfony 2.2-beta2 is out later this week. This takes care of the API change, so let's get it through now to keep things simple.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

This doesn't apply for me.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Needs review

#5: 1894508-03-update-serializer.patch queued for re-testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
1894508-11-update-serializer.patch52.09 KB

Merged in the changes from a routing update and updated the XMLEncoder which was committed today.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
1894508-11.patch52.88 KB

Rerolled and included what was committed earlier in #1854874: Add serializer support for XML.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Doh! sorry, x-post on the reroll there. I guess we can compare...

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Lin, shall we go with your patch in #11 due to the routing changes you made?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

No, I don't think there's a functional difference there. Your patch follows our code style better, so lets go with #12.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

Since Crell already RTBCed this and it only needed a reroll, I'm going to re-RTBC it.

Patch in #12 should be used.

Crell’s picture

Crell CreditAttribution: Crell commented

Re-confirming my RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Fixed » Needs work

Sorry, I lied. #1894508: Update symfony/serializer was 700K + a critical, so it won the great battle of conflicts. I reverted this commit, so this will need a re-roll now.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
1894508-20.patch60.38 KB

Stop playing with our emotions like that! :)

Rerolled.

scor’s picture

scor CreditAttribution: scor commented
FileSize
1894508-21.patch61.39 KB

#20 looks good. this patch pins the serializer dependency to beta2

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC, since the combined pinning patch is still breaking.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committin' this while the committin' is good. :D

Committed and pushed to 8.x!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Update symfony/serializer » HEAD Broken: Update symfony/serializer
Priority: Normal » Critical
Status: Fixed » Active

I'm reasonably sure this is what broke HEAD. It changed the autoloader which would explain the fails.

Not in front of a computer to test though.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: HEAD Broken: Update symfony/serializer » Update symfony/serializer
Priority: Critical » Normal
Status: Active » Fixed

core is green again

Automatically closed -- issue fixed for 2 weeks with no activity.