Problem/Motivation
While working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, I discovered a rather nasty problem with the xml
format. On the (de)normalizer side, not the encoder/decoder side.
Steps to reproduce
-
$serialized_test = $this->serializer->serialize($node, 'xml'); $unserialized_test = $this->serializer->deserialize($serialized_test, get_class($node), 'xml');
-
- Expected result: That should serialize a
Node
entity to thexml
format and deserialize it again. - Actual result:
Symfony\Component\Serializer\Exception\UnexpectedValueException: A string must be provided as a bundle value.
, thrown atcore/modules/serialization/src/Normalizer/EntityNormalizer.php:74
.
- Expected result: That should serialize a
With an explicit example:
- Serializing (PHP array -> XML)
['field_name' => [['value' => 'something']]]
-><field_name><value>something</value></field_name>
- Deserializing (XML -> PHP array)
<field_name><value>something</value></field_name>
->['field_name' => ['value' => 'something']]
(note how this got rid of the "items" level and assigned first item as the complete value of the field)
In other words: PHP array -> XML -> PHP array does not result in two equal/identical arrays! Information is lost.
For failing test, please await #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method see #3 (unit test) and #88 (functional test).
Proposed resolution
- Proposal 1 (Wim Leers in #12):
Removexml
format and all associated code - Impossible, because there may be sites out there that rely on it.
- Proposal 2:
Removexml
format, but keep the underlying base classes - Because there are custom projects that extend the base classes to add their own XML-based formats — see #21 and #2824837: Change the root node name of a custom REST resource's XML encoding as an example.
- This was RTBC'd in #54 by
serialization.module
maintainer @damiankloip. - Impossible, because there may be sites out there that rely on it — just reading XML responses works fine
- Proposal 3:
Removexml
format for general use, but keep it for REST export views that use XML, and deprecate the underlying base classes - Because this minimizes disruption.
- This was proposed by @damiankloip in #60 after @alexpott un-RTBC'd proposal 2 in #57. Wim Leers RTBC'd this in #68.
- Impossible because it also breaks other use cases that work, such using cookie-based REST authentication by sending a
xml
request body, raised by @alexpott in #73. - Proposal 4: #93 + #94 + #95
- Do something similar to what we do for the HAL normalization:
\Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait
but it instead do it for XML encoding quirks that remain after decoding, and hence are still present in the normalization, but should not.In other words: be pragmatic, and accept that PHP array -> (encoding) -> XML -> (decoding) PHP array is a lossy operation (the arrays at the beginning and end will be different). That doesn't mean nobody can use it. So rather than removing XML support altogether, just test the generated XML output, warts and all. This way we can at least prevent regressing in our XML support!
We never supported writing (denormalizing) XML, but at least we can support reading XML (normalizing). That's what this proposal does.
100% test coverage (for all entity types) in #128.
- #2905655: XML encoder does not support UUIDs as keys: makes ImageStyle config entity XML serialization crash
- #2905686: Expand EntityResourceTestBase to test PATCHing multi-value fields: both adding and removing items
The first 3 proposals were shot down, the 4th is the one that fulfils all requirements!
Remaining tasks
Follow-ups:
User interface changes
TBD
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#166 | 2800873-166.patch | 168.12 KB | Wim Leers |
#67 | interdiff-2800873-67.txt | 1.69 KB | damiankloip |
#67 | 2800873-67.patch | 9.93 KB | damiankloip |
#62 | interdiff-2800873-62.txt | 1.45 KB | damiankloip |
#62 | 2800873-62.patch | 9.15 KB | damiankloip |
Comments
Comment #2
Wim LeersFailing test at #2737719-13: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.
Comment #3
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedHere is a failing test for the serialization module, in the 'integration' type test, EntitySerializationTest. We should not rely on REST module coverage, not sure how it was not tested in here already. ha. We just test serialization, and deNORMALIZATION! Face palm.
So basically what happens here, is the Symfony XML encoder will serialize array structures like
['type' => [['value' => 'stuff']]]
to<type><value>stuff</value></type>
. So the additional level of nesting that fields us is lost. So when this is decoded again, you end up with['type' => ['value' => 'stuff']]
..This looks to be the completely intended behaviour from their perspective, see
\Symfony\Component\Serializer\Encoder\XmlEncoder::buildXml
:Comment #4
neclimdulBah, XML sucks... Symfony expects this to come in as
['type' => [['value' => 'stuff'], ['value' => 'think']]]
so it would then get multiple values it could get it back into an array. XML just isn't a good match for loosely defined data structures :(Comment #5
Wim LeersRetitling accordingly, to indicate this is a generic problem.
Comment #6
Wim LeersAlso note that this problem then must apply to every single field, because
'field_name' => [['value' => 'the value']]
is what every field's PHP data structure looks like.Hence the rather ominous sounding
in the title.Comment #7
tstoecklerWell, one could also say that Field API is just being annoying in the case of the bundle field. #2443165: Drupal\Core\Entity\EntityInterface\ContentEntityStorageBase::doCreate() assumes that the bundle is a string would fix this, correct?
Comment #8
Wim LeersNo. This is a generic problem, because all fields have multiple items.
In other words:
['field_name' => [['value' => 'something']]]
-><field_name><value>something</value></field_name>
<field_name><value>something</value></field_name>
->['field_name' => ['value' => 'something']]
(note how this got rid of the "items" level and assigned first item as the complete value of the field)In other words: PHP array -> XML -> PHP array does not result in two equal/identical arrays! Their structure is completely different.
Comment #9
tstoecklerAhh, thanks. So the bug is in fact in the serializer. In only surfaces for the bundle field due to #2443165: Drupal\Core\Entity\EntityInterface\ContentEntityStorageBase::doCreate() assumes that the bundle is a string. And I guess fields with multiple field items will lose anything but the first field item.
Or one could also say that non-bundle single-value fields happen to work because of Field API's flexibility in accepting input values.
Comment #10
Wim LeersTracking the issues that #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method uncovered, by making them all child issues.
Comment #11
neclimdulThis is probably going to be particularly difficult since Symfony's implementation is by design a trivialized solution. XML though expects you'll have some understanding of the structure through something like a DTD or some other definition structure though. For example, jms/ serializer is a more complete serialization library and it has annotation, XML and JSON definition structures hooked into the deserializer that allow it to specify the structure.
Comment #12
Wim LeersI vote we remove support for the
xml
format. Precisely because it is clearly broken, it seems like this would be safe to do, because otherwise this bug would've been reported years ago.damiankloip, maintainer of the
serialization.module
component, is at least not firmly opposed:Comment #13
Wim LeersThis does not remove a lot of code, but does remove a lot of maintenance work. And that's not not even considering the enormous amount of work that would be necessary for fixing this deep-rooted problem, which would require BC breaks anyway as well as boatloads of test coverage.
Comment #15
yogeshmpawarI have rerolled the patch to apply with 8.2.x branch.
Comment #16
neclimdulThis could be a disruptive change. 8.2 might be a bit much since its in RC so lets target 8.3 for now.
Comment #17
Wim LeersSure, but:
I'm fine with leaving it in its current status during 8.2.x.
Comment #19
yogeshmpawarSame patch testing against 8.3.x branch.
Comment #21
Wim Leers#2824837: Change the root node name of a custom REST resource's XML encoding was just filed and answered. So there is a valid need for custom XML formats. So we should leave
Drupal\serialization\Encoder\XmlEncoder
in place and mark it deprecated — we should just remove the service & tests, so that it cannot be configured any longer.The remaining failures here simply are happening because not all relevant test coverage has been removed yet.
Comment #22
Wim LeersComment #23
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #24
Wim LeersWoot! Thanks vprocessor! I'll provide reviews!
Comment #25
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedYes, I am happy with that approach also. Custom XML formats should definitely be supported.
Comment #26
lhangea CreditAttribution: lhangea commentedvprocessor please let me know if you are still working on it. I can help here if needed.
Comment #27
Wim Leers@damiankloip yay! Very glad to have your support.
@lhangea he hasn't reported back in 5 days, and hasn't responded to you in almost 24 hours. I think it's fine for you to take over from him. If it turns out he's worked on this in the mean time, he'll be able to do an excellent review of what you've done :) So, unassigning him. Go ahead now :)
Comment #28
lhangea CreditAttribution: lhangea commentedtomorrow I will be on my way to IronCamp so maybe the next day I will be able to work on it :)
Comment #29
Wim LeersCool! :)
Comment #30
lhangea CreditAttribution: lhangea commentedLet's see with this patch. Actually I'm not sure if we need to completely remove the StyleSerializerTest::testResponseFormatConfiguration - like I did in the patch - or we should just remove parts of it.
Comment #32
lhangea CreditAttribution: lhangea commentedComment #33
vprocessor CreditAttribution: vprocessor at Skilld commentedSorry guys, was too busy :-(
Comment #35
lhangea CreditAttribution: lhangea commentedComment #36
lhangea CreditAttribution: lhangea commentedthe test fail is the one described here: #2157927: Intermittent test fails in LocaleUpdateTest::testUpdateImportSourceRemote()
Comment #37
Wim Leers@lhangea: Looking good! :) Thanks!
@vprocessor: No worries! We all get unexpectedly busy :)
This is test coverage we lose, but I think it's low-risk anyway. We've got lots and lots of test coverage for this elsewhere.
This should not be deleted altogether. The relevant portions should be kept.
This needs to be more specific.
Hm, this is losing significant test coverage.
Rather than going from
['json', 'xml']
to['json']
, let's go to['json', 'hal_json']
. That will mean adding thehal
module in this test, but that's fine.That will require us to first fix #2820888: Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order though. Fortunately, the fix was already found, it just needs a bit of test coverage… and that test coverage can be simply expanding
['json', 'xml']
to['json', 'xml', 'hal_json']
in that issue, and then removing'xml'
again in this issue :)Comment #38
Wim LeersComment #39
Wim Leers#2820888: Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order has landed, this is now unblocked :)
Comment #40
lhangea CreditAttribution: lhangea commentedThanks Wim for feedbacks. I tried to address them in this patch.
For #2 I tested with json and hal_json instead of xml and json like it was before.
Comment #41
lhangea CreditAttribution: lhangea commentedComment #44
lhangea CreditAttribution: lhangea commentedAll the remaining tests probably happen because of '415 when request body is existing but not allowed format' tests from
EntityResourceTestBase
I see that for that portion of code there is also something to do from #2826407: PATCH + POST allowed format validation happens in RequestHandler::handle(), rather than in the routing system. Maybe the changes don't necessarily depend on each other but as far as this issue is concerned should we replace that xml content type header with hal_json ?
Until I get some feedback on this issue I commented out those 2 tests to check if the patch actually passes if we solve them properly.
Comment #45
Wim LeersThis is what is key about this. This issue is removing the
xml
format, but we can easily add a test-only format.So, let's do that.
EntityResourceTestBase
is already installing therest_test
module. Let's let that module add a newfoobar
format (which doesn't need to actually exist, it just needs to be registered, so that these tests can work), and use that instead ofxml
inEntityResourceTestBase
. Problem solved!Once that's done, this is ready! Thanks for pushing this forward :)
Comment #46
lhangea CreditAttribution: lhangea as a volunteer commentedI registered the MIME Type but doesn't seem to work - at least on my local tests it keeps filtering the good route and throwing a
UnsupportedMediaTypeHttpException('No route found that matches Content-Type: ...
inContentTypeHeaderMatcher
.But let's try it anyway here, maybe something's wrong with my local.
Comment #48
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedI think this is registered too late. IIRC you will need to register this in a ServiceProvider instead and add code this in there:
EDIT: Yes, just checked. See HalServiceProvider for a good example :)
Comment #49
Wim Leers#48 is right. Done. But there's one extra small bit you need to do for a format to show up in the
serializer.formats
container parameter: provide a service that\Drupal\serialization\RegisterSerializationClassesCompilerPass
will pick up.Comment #51
Wim LeersOops. This is the right patch.
Comment #53
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedAh. Yes - I thought the patch already had that part for some reason. Nice!
Comment #54
damiankloip CreditAttribution: damiankloip at Acquia commentedThis looks good to me now. I checked it out locally. The remaining usages of the XML format in the serialization module are in a couple of tests, which still makes sense to have. E.g. in
\Drupal\Tests\serialization\Unit\Encoder\JsonEncoderTest
:I will do the CR today.
Comment #55
damiankloip CreditAttribution: damiankloip at Acquia commentedCR: https://www.drupal.org/node/2835098
Comment #56
Wim LeersYAY!
Comment #57
alexpottI'm not sure we can do this - isn't it perfectly possible to create an XML view that works as you would like it to in Drupal 8?
There's no upgrade path for existing views using xml. What happens to them?
Why wouldn't we leave this test coverage here if we are deprecating the class - we still need to test it.
Comment #58
damiankloip CreditAttribution: damiankloip at Acquia commentedHmm, good questions. If that's the case then all we should/could really do is just add a deprecation message to this class and be done?
I should have considered the views serialization case a bit more really, as I was the one who added it all originally... :P For existing views to work we would absolutely have to keep the defined format for serialization.formats.
Comment #59
Wim LeersSo something that was never given the necessary test coverage and is clearly completely broken when you look at the full set of things it supports, actually works fine if you look at a small subset of the things it supports, namely some XML views.
This is painful. :(
So, how can we allow
RestExport
View displays to still usexml
, without allowing thexml
format? Keeping the encoding is okay, but thexml
format is not. Remember: format = normalization + encoding.This must absolutely be removed.
So that leaves us no choice but to hardcode XML support in
\Drupal\rest\Plugin\views\style\Serializer
somehow.Comment #60
damiankloip CreditAttribution: damiankloip at Acquia commentedNot sure what you mean about "format = normalization + encoding." ? We need normalization otherwise encoding cannot work properly anyway. Either way, we don't have any XML specific normalizers that I'm aware of so that shouldn't affect much. It is only when the serializer comes to encode the format that it will be unhappy. So yes, in a nutshell, we need to keep the XmlEncoder with the 'name' tag (encoder) but not the 'format' attribute.
So in that case, it probably makes sense to keep most of the test coverage. This brings that back. With a couple of small changes needed for it to still work without the registered serialization format:
We need to manually hardcode the XML format here so it is still an available option in the UI and when rest export routes are built.
We need to keep the encoder tag so it can still be used to encoding by the serializer, format is removed so it is not registered as a serialization format everywhere.
Small change here to allow no format.
Comment #61
Wim LeersI was referring to https://www.drupal.org/developing/api/8/serialization and http://symfony.com/doc/2.7/components/serializer.html. You can see the distinction most clearly in our
hal_json
format: it uses theapplication/hal+json
MIME type, which uses thehal
normalization and thejson
encoding. Combine that normalization and that encoding and you end up with theHAL+JSON
format.I was under the perhaps mistaken impression that Views RestExport displays only use the
xml
encoding, not thexml
format.Well, no, I can't be mistaken I think, because
\Drupal\rest\Plugin\views\style\Serializer::render
looks like this:i.e.
\Drupal\rest\Plugin\views\row\DataEntityRow
will use the existing entity normalizers, and will then run into the same limitations/bugs wrt serializing entire entities to XML. But in the case of\Drupal\rest\Plugin\views\row\DataFieldRow
, we only use thexml
encoder, which would hence be able to avoid those limitations/bugs.Sounds good!
I think this also allows new REST Exports to be created. Should the comment be updated to reflect that?
s/deprecated/unsupported/
Should mention 8.3.x specifically.
Comment #62
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks, nits addressed.
RE the format vs encoding, I think I see what you mean now. I am not sure we have any way around that at this point if we want to maintain what people are currently getting. If they are not using multi value field for example, that data is probably usable. I don't think we want to go down the route of trying to normalize entities for XML ourselves or anything.
Comment #63
Wim LeersExactly. As soon as they have a multi-value field, this will break down horrendously, for the same reasons. So, let's document that in
\Drupal\rest\Plugin\views\style\Serializer::render()
as a known, unfixable bug. Then our future selves won't be pulled down this rabbit hole again :)Comment #64
damiankloip CreditAttribution: damiankloip at Acquia commentedThat sounds reasonable :) I'll add a comment to that effect.
Comment #65
Wim LeersIMHO back to RTBC once that comment is added.
Comment #66
Wim LeersComment #67
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is a comment added.
Will REST routes that are already configured to allow XML continue to work? Not sure if we need a little more discussion around that side of things in this issue?
Comment #68
Wim Leers@damiankloip and I discussed that on IRC yesterday:
Comment #69
damiankloip CreditAttribution: damiankloip at Acquia commentedok, works for me. Let's see what the 8.x maintainers think I guess :)
Comment #70
Wim LeersYep, exactly!
Comment #72
damiankloip CreditAttribution: damiankloip at Acquia commentedI think this is ok again (CI fail).
Comment #73
alexpottThis feels really tricky to break for the same reasons as breaking views. If someone is doing xml logging in we're just going to break it :( And we ha ve test coverage showing it works.
To be honest I think we should just throw an explicit exception when we hit this issue and somehow try to discourage it. And also consider what we should do for Drupal 9.
Comment #74
Wim LeersI'm the one who insisted back in #2403307: RPC endpoints for user authentication: log in, check login status, log out that we add test coverage for >1 format. All of the "old" REST test coverage was only testing with a single format, which led to lots of unknown problems. So I insisted we tested with the two formats we had:
json
andxml
(because that would mean that that test would not need to install thehal
module too). Only in the past 2 weeks did we addhal_json
.In other words: if I didn't insist on better test coverage back then, you wouldn't have raised this concern.
Continuing to support
xml
is a lie, because we cannot support it, because it cannot work, see the issue summary. The chances of somebody actually using this are extremely slim, because if somebody were usingxml
, then they'd have reported the problems described in the IS already. The fact that Drupal 8 has been out for more than a year and not a single person has reported problems with XML clearly implies that nobody is using XML.So let us please stop supporting it. Because there's no sane way to support it. Yes, we made a mistake in the past by shipping the REST module as "stable" while it really wasn't. Yes, we made a mistake in shipping with extremely poor test coverage. But please don't let that mean supporting those mistakes forever, because there's no reasonable way to support it.
Comment #75
alexpottWell just because committers might have missed something doesn't mean that we should ignore the BC concerns. Two wrongs and all...
I agree that is was a mistake making REST stable when we did, but also we have to face the fact that you can use XML to log in to a Drupal atm in HEAD. This patch removes that support. At the very least the change record needs updating but I also think the release managers need chime in on this issue. It'd be great if the issue summary and change record can be updated with all of the effects of this change.
Comment #76
Wim LeersThat's fair!
But I think it's also fair to then say as the only active maintainer of the REST module that I will plainly refuse to support any XML-related issue. I will mark them
and point to this issue. Because there is no reasonable way for me to provide support, except for to suggest to switch to another format.That's also exactly why this issue exists by the way: for maintainability/supportability. I could've chosen to ignore this and not open an issue for it (I opened this while working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, in which I tried to provide full test coverage for the
xml
format, and was unable to because of this). But I want to ensure that we can provide support. And in the case of thexml
format, that's unfortunately just not possible.Comment #77
dawehnerI'm wondering whether its maybe totally okay for people using XML support to just serialize XML but never send it back to Drupal. This usecase actually kind of works, doesn't it?
Comment #78
Wim LeersGETting any entity with a multi-value field that actually has multiple values is broken.
Comment #79
dawehnerI try to understand why this is broken ...
The resulting XML looks like the following:
This for itself is a perfectly valid XML, which you can process:
Of course it is a problem that unserialization doesn't work, but this doesn't necessarily mean noone is able to use the current state of quo for some usecases at least.
Do you mind lighten me up?
Comment #80
damiankloip CreditAttribution: damiankloip at Acquia commentedRight, so that main problem is that it doesn't actual represent the structure of the data we have. So in your example above, it's true, that could be totally usable for something consuming that data, just not a Drupal 8 site (unless they have some custom code to parse it). The seems be a concoction of XML being a bad format for this kind of data, and Symfony XML encoding making some further assumptions (to add children to the parent for empty arrays, for example). To try and fix this properly would take a lot of work, for little reward. And even then, it may still not be a done deal. I think that's the general nutshell version :)
Comment #81
dawehnerI try to understand why this isn't the case. If I look at the XML example from above, it really looks like valid xml. It has both the field item and field item list level. While I totally agree, we don't have to support denormalization, I somehow believe we should keep the existing code out there, you know, people might rely on it. XML export into other systems are still a common thing.
Comment #82
Wim LeersBut that's the thing: we just remove the default
xml
format, because it has the flaws described in #80 and before. We do NOT prevent people from creating custom XML-based formats to generate data to be used in other systems.Hence this. Which means we don't break any existing code that has this class/service injected, but we remove the
xml
format from being available for REST resources.But also this! This means you can still create custom XML-based formats. without having to reimplement everything.
Comment #83
dawehnerSorry for my stupidity. Here is the scenario I have in mind:
Site A uses a XML resource and export its content to a custom script, which is able to parse exactly their custom data.
When this site updates to 8.3.x, it stops working? This is a BC for me at least. Maybe we could add a BC layer to don't expose it by default or so?
Comment #84
damiankloip CreditAttribution: damiankloip at Acquia commentedSo with this patch, it should continue to work, no problems. I wonder whether the other changes are also going to not be allowed though. Maybe we are tied here to just marking the classes as deprecated?
Comment #85
dawehnerWell, for me the only sane steps from a BC point of view are
Comment #87
Wim LeersLet's finally get this resolved. No matter what happens (XML support removed or not), we should get this issue on track. Letting this linger is bad for everybody: users and us core developers.
First of all: see the STR in the IS plus comments #2 through #11 about the fundamental problems. The XML serialization clearly is unable to retain arrays, which is the root cause of all these problems.
Yes, this continues to work.
This means REST views that use XML continue to work.
This would mean that any REST resources that use the XML format will no longer be able to use the XML format, even if they specify it. This is the only BC break.
That's of course where @dawehner is right when he says
It is possible people are using this, but if they are, they can only be using it for
GET
orDELETE
, not forPOST
orPATCH
.So then here's the thing I propose: we explicitly document that the XML format is unsupported. It will receive zero support. It will be confusing that it still exists but is unsupported, but so be it. We could then also do something like: add an update hook that iterates over all REST views and over all REST resource config entities, check if
xml
is present in any of them, and store that resulting value instate
. We then have aServiceModifierInterface
implementation that reads this value from state and removes theformat: xml
portion of theserializer.encoder.xml
service.That would mean only sites that have used the
xml
format would still be able to use it. This would reduce the maintenance burden.Finally: if you do want to retain XML support, then please work to get this patch to pass. This applies all REST test scenarios to the
EntityResource
REST resource, for theNode
entity type, in thexml
format.Comment #88
Wim LeersImproving the test coverage that you'd have to get to pass.
Now you can clearly see (in the failures for
NodeXmlAnonTest
) that:GET
has a normalization that is incorrect (loses array structure)POST
andPATCH
both fail because the request body cannot be denormalized — if you debug this, you'll see it has the exact same error message as the one I reported in the ISDELETE
works because there's no normalization to send nor receiveComment #91
Wim Leers#88 confirmed exactly in the failed test output:
1 = GET
2 = POST (400 because bundle is missing)
3 = PATCH (400 because bundle is missing)
Comment #92
dawehnerWell, this causes issues with installation profiles, doesn't it?
Comment #93
Wim LeersIS completely rewritten, to get this issue back on the rails. In it, I also summarized the previous proposals, and why there were considered ineligible for commit.
I also took another good hard look at what @dawehner has been saying in #79, further explaining in #81, #83 and #85. Sorry Daniel, for not taking the time to truly digest what you were saying. Sorry for letting my frustration with the state of Serialization/REST result in stubborn comments :(
Patch in progress to attempt to do what @dawehner suggested: test coverage for XML
GET
support. Already spent a few hours on it, hopefully almost there! Hence also removing the tag.Comment #94
dawehnerNo worries, this happens to all of us.
I have another question, maybe really naive one. Could we theoretically fix the decoding process? For example we could annotate the XML with multiple=TRUE to help the decoder, but I agree supporting a format which almost noone uses feels weird.
Comment #95
Wim LeersHere's updated test coverage that makes it work for
Node
+ XML. For anon + basic auth + cookie!Best of all: it should pass! 😵🙏🎉
It's doing something similar to what
\Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait
does for the HAL normalization, but it instead does it for XML encoding quirks that remain after decoding, and hence are still present in the normalization, but should not. In other words: it's pragmatic.Can't wait to get reviews!
Comment #96
Wim LeersBetter title to reflect #95's direction.
Comment #97
Wim LeersNote: we still need to add test coverage for all other entity types!
Comment #99
Wim LeersI can't reproduce the new XML test classes' failures :(
Sadly, the output at https://www.drupal.org/pift-ci-job/713188 is truncated … and https://dispatcher.drupalci.org/job/drupal_patches/21624/consoleFull does not contain the full output either.
This is going to be difficult to fix…
Comment #100
Wim LeersThis reverts the changes to
EntitySerializationTest
that was in the patches from months ago — which was adding test coverage for deserializing XML, which we're no longer doing.Should bring it down to 3 failures. All in the 3 new
NodeXml*Test
classes.Comment #101
Wim LeersThese changes are no longer necessary, because we're not testing POST or PATCH for XML.
This then also fixes the 2 only coding standards violations that DrupalCI is reporting.
Comment #104
dawehnerWe should document this if potentially?
It would be nice to maybe have a comment about this one. It feels like this makes it impossible to have multivalue fields, right?
Nice!
Comment #105
Wim LeersAlso:
But at least we have a starting point now!
Comment #107
Wim LeersLet's get this going again. Let's first reupload #101, because AFAICT it's passing locally now, without any further changes.
Comment #109
Wim Leers#104
I think that with this, our XML test coverage is complete enough for read-only purposes. Next step (and last step 🎉): expand this to all other entity types!
Comment #110
Wim LeersI noticed a bug in #109, which will cause every config entity test to fail.
Comment #112
Wim LeersSo the same failures still exist in #107 as in #101.
Sadly, the error output is truncated… but thanks to the downloadable build artefacts I still can access it :) Turns out there is one difference: a
Vary: Accept-Encoding
header is present. Apparently only for XML. It must be added by testbot's web server, because I can't reproduce it. In any case, that's definitely a header that's safe to ignore in our tests — since the failing test is about asserting that the response headers are the same for GET and HEAD requests. If testbot's web server is adding aVary
request only for XML responses to GET requests, there's nothing we can do about that.So this simple measure should fix that :) Hopefully this comes back green!
Comment #114
Wim LeersYay! Now: expand this to all other entity types!
Comment #115
Wim LeersAdding
Comment
(the alphabetically first content entity type).Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #116
Wim LeersAdding
Feed
(the alphabetically second content entity type).I had to update
XmlEntityNormalizationQuirksTrait::applyXmlFieldDecodingQuirks()
to special-case theListIntegerItem
andTimestampItem
field types' normalizations XML decoding quirks.Comment #117
Wim LeersAdding
Item
(the alphabetically third content entity type).Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #118
Wim LeersAdding
MenuLinkContent
(the alphabetically fourth content entity type).I had to update
XmlNormalizationQuirksTrait::applyXmlDecodingQuirks()
, because it was not yet working in a recursive manner: it was only handling the first level. This is likely the last time this method needs to be updated.Comment #119
Wim LeersAdding
Shortcut
,Term
andUser
. All remaining content entity types for which REST test coverage exists (but only JSON and HAL+JSON, not XML).Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #120
Wim LeersAdding
Action
(the alphabetically first config entity type).I had to add
XmlEntityNormalizationQuirksTrait::applyXmlConfigEntityDecodingQuirks()
.Comment #121
Wim LeersAdded
BaseFieldOverride
+Block
+BlockContentType
+CommentType
+ConfigTest
+ConfigurableLanguage
+ContentLanguageSettings
+DateFormat
(all config entity types, with more yet to come).Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #122
Wim LeersAdded
Editor
.This one was pretty tricky. Because
\Symfony\Component\Serializer\Encoder\XmlEncoder::buildXml()
does all sorts of special things to nested arrays of a particular kind:This means that you end up not with
but with:
(Note one level less in the array, and the use of
@key
.)Comment #123
Wim LeersAdded
FieldConfig
+FieldStorageConfig
+FilterFormat
.Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #124
Wim LeersAdded
ImageStyle
.Ran into a problem again. Interestingly, it works fine for the JSON and HAL+JSON formats. But for XML, this is the response you get:
The root cause:
i.e. a UUID is being used as a key for an array, which means that the XML encoder will try to map this to an XML tag (element). And dashes aren't allowed in XML tag/element names, hence this exception.
I don't see a way to work around this. But it also shouldn't hold up everything else. So created a new issue for that: #2905655: XML encoder does not support UUIDs as keys: makes ImageStyle config entity XML serialization crash.
Therefore for now including stub test coverage.
Comment #125
Wim LeersAdded
Menu
+NodeType
+RdfMapping
+ResponsiveImageStyle
+RestResourceConfig
+Role
+SearchPage
+ShortcutSet
.Zero additional changes were necessary — I just had to add the necessary test classes :)
Comment #126
Wim LeersAdded
Tour
.This time there is another XML decoding quirk that wasn't yet taken into account.
Comment #127
Wim LeersAdded
View
+Vocabulary
. Now all config entity types are done too.Zero additional changes were necessary — I just had to add the necessary test classes :)
I'd say I'm 100% done, but I noticed I forgot the
EntityTest
+EntityTestLabel
content entity types earlier. Doing those next, then this is ready & RTBC'able.Comment #128
Wim LeersAdded
EntityTest
+EntityTestLabel
.Test coverage complete!
Comment #129
Wim LeersUpdated IS. Reviewed the patch myself:
This is a debug leftover that should be deleted.
Comment #130
Wim LeersSince this issue is now 100% test coverage, it can actually still be committed to 8.4.x too (queued 8.4 test for #129). And it's not a bug report, but a task.
Comment #131
dawehnerNote: I'm using emoji based code review, see https://gist.github.com/pfleidi/4422a5cac5b04550f714f1f886d2feea
🤔 Having to put that in all those tests is a bit not ideal ... but I guess that's necessary evil right now. ❓... why do we not exclude those automatically in
XmlEntityNormalizationQuirksTrait
?🤡 Do we actually test updating of a multivalue field? I guess this is out of scope of this issue.
❓This is weird ... one the one hand we throw an exception and on the other hand we handle the other case. I guess the exception message is wrong?
🤡 According to https://www.w3.org/TR/2001/REC-xmlschema-2-20010502/ it should be totally possible to have typed XML, but yeah let's not care about this here.
Comment #136
Wim Leers#126 fixed something for
Tour
, but caused a regression forResponsiveImageStyle
. Fixing that.Comment #137
Wim LeersFixed coding standards violations.
Comment #138
Wim Leers#131: 🙏❤️
in
XmlEntityNormalizationQuirksTrait
. The two overrides you are quoting areUserResourceTestBase
-specific tests.Comment #139
Wim LeersAlso, this current patch means the draft CR at https://www.drupal.org/node/2835098 could be deleted :)
Comment #141
dawehnerYeah see my used emoji :)
Oh I see, this makes sense :)
Comment #142
damiankloip CreditAttribution: damiankloip at Acquia commented+1 on this RTBC, super thorough - looks great.
Alas, this is one of our big issues with XML :/
Comment #144
Wim LeersRandom fail in
SettingsTrayBlockFormTest
(JS test).Retesting.
Comment #145
Wim LeersSince #138 was RTBC since August 31, the following issues have landed, which added more test coverage that should also get XML test coverage:
ContactForm
: #2843778: EntityResource: Provide comprehensive test coverage for ContactForm entityEntityTestBundle
: #2843776: EntityResource: Provide comprehensive test coverage for EntityTestBundle entityBlockContent
: #2835845: EntityResource: Provide comprehensive test coverage for BlockContent entityMedia
+MediaType
: #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity typesUpdated the patch to add XML test coverage for all of those too! Keeping at RTBC because there's nothing tricky there — it's all exactly like the existing patch :)
Comment #147
Wim Leers#2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity only landed for 8.5. Which means this is now an 8.5-only issue.
Comment #148
larowlanHey all, can we get an issue summary update - it still lists 4 possible solutions?
Comment #149
Wim LeersThe IS is up-to-date, actually. The first 3 are struck through with
<del>
, the fourth proposal is not. Clarified this.Comment #150
Wim LeersThis blocks #2868035: Test that all core content+config entity types have functional REST test coverage.
Comment #151
Wim LeersSince #145, the following landed:
Updated the patch to add XML test coverage for all of those too! Keeping at RTBC because there's nothing tricky there — it's all exactly like the existing patch :)
Furthermore, this:
::setUp()
testPatchPath()
method that was added toNodeResourceTestBase
andTermResourceTestBase
Now only a single REST test coverage issue remains: #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler, which is unlikely to land anywhere in the next few weeks. Which means this patch is unlikely to need to expand in the next several weeks.
Comment #152
dawehnerJust a drive by comment. The test coverage looks great for me!
An idea for a future follow up: Ensure that adding/removing items from the list works.
Comment #153
Wim LeersYou suggested something similar/related in #131.2, which I created an issue for in #138: #2905686: Expand EntityResourceTestBase to test PATCHing multi-value fields: both adding and removing items. Updated/clarified that issue now to also include what you suggested in #152. 🙂
Comment #155
dawehnerMy brain is porous, sorry for that :)
Comment #156
Wim LeersThis was RTBC for >2 weeks, and now got un-RTBC'd due to a random failure. 😡😡😫😫 Retesting, back to RTBC.
@dawehner: no worries, you could not possibly have remembered that!
Comment #158
Wim LeersHm, this started segfaulting apparently. Retesting.
Comment #160
Wim Leers#2901562: Fix 'Squiz.WhiteSpace.SuperfluousWhitespace' coding standard landed yesterday and caused a conflict. Easy conflict resolution.
Comment #161
larowlanbe => be added? can be fixed on commit
Just for my understanding - we're only adding tests here? I was expecting to see something changed outside tests - but its just for tests?
Comment #162
Wim LeersCorrect. We're adding
<EntityType>Xml<AuthProvider>Test
classes and we're expanding the base test coverage to explicitly test multi-value fields.I understand why you think that, because that was originally the direction/title of this issue.
See the IS: I was convinced we had to thoroughly change the XML encoding process, which would be a BC break. But most people that use the XML format, only use it for reading (
GET
), not writing (PATCH
&POST
). Turns out that for that, it's actually perfectly workable.So, as the IS says, as of #93 + #94 + #95, this issue changed course from "OMG XML support is completely broken, let's remove/deprecate it" to "reading XML is not wonderful but it kinda works, so let's add test coverage to ensure we don't break BC".
Consequently, this patch is indeed only changing tests.
Fixed.
Comment #163
Wim Leers#2868035: Test that all core content+config entity types have functional REST test coverage just landed. Rerolled this issue, to ensure we have XML REST test coverage for all entity types. That resolves a
@todo
that that issue added, which pointed to this issue.It should've been as easy as removing one
@todo
and adding the three XML suffixes (XmlAnonTest
,XmlBasicAuthTest
andXmlCookieTest
). But then I realized that the error generated byEntityResourceRestTestCoverageTest
was not very helpful. It only indicated whether test coverage was incomplete in some way. It didn't say which specific areas were missing (e.g. JSON + Basic Auth or XML + Cookie). So I made some additional changes.On the bright side, this did uncover that in fact this patch was still missing XML test coverage for
workflow
:That's the failure that should show up in the test results for this patch. Which also handily means that this patch is now proving that it is adding complete XML REST test coverage!
Comment #164
Wim LeersAnd this then adds the trivial
WorkflowXml*Test
test coverage, which makes the patch green again.Comment #166
Wim Leers#163 and #164 yielded exactly the expected results :)
This is also adding two coding standards violations:
Fixed those.
Interestingly, PHPStorm failed to mark that unused use statement as unused. PHPStorm--; PHPCS++.
Comment #167
larowlanAdding review credits
Comment #169
larowlanFixed!
Committed as c53b64f and pushed to 8.5.x.
:tada:
Comment #170
Wim LeersOMG OMG OMG YAY! <3
Deleted the CR at https://www.drupal.org/node/2835098 from December 2016, because it's no longer correct: we're not deprecating the
xml
format, we're now just adding test coverage to prove it works in a limited fashion. Now we'll finally know when we break BC :)This took more than a year, but now we can finally stop worrying! :)
Issue updates:
Comment #171
Wim LeersNow all follow-ups have patches:
Comment #172
Wim LeersCreated a new follow-up, for making this problem go away in D9: #2926034: Deprecate 'xml' serialization format in Drupal 10.