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

  1. $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 the xml format and deserialize it again.
    • Actual result: Symfony\Component\Serializer\Exception\UnexpectedValueException: A string must be provided as a bundle value., thrown at core/modules/serialization/src/Normalizer/EntityNormalizer.php:74.

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): Remove xml format and all associated code
Impossible, because there may be sites out there that rely on it.
Proposal 2: Remove xml 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: Remove xml 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.

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.

CommentFileSizeAuthor
#166 2800873-166.patch168.12 KBWim Leers
#166 interdiff.txt1.74 KBWim Leers
#164 2800873-164.patch168.17 KBWim Leers
#164 interdiff.txt3.03 KBWim Leers
#163 2800873-163.patch165.21 KBWim Leers
#163 interdiff.txt3.33 KBWim Leers
#162 2800873-162.patch161.95 KBWim Leers
#162 interdiff.txt1.32 KBWim Leers
#160 2800873-160.patch161.94 KBWim Leers
#151 2800873-151.patch161.87 KBWim Leers
#151 interdiff.txt20.99 KBWim Leers
#145 2800873-145.patch144.38 KBWim Leers
#145 interdiff.txt18.11 KBWim Leers
#138 2800873-138.patch127.96 KBWim Leers
#138 interdiff.txt1.58 KBWim Leers
#137 2800873-137.patch128.25 KBWim Leers
#137 interdiff.txt1.58 KBWim Leers
#136 2800873-136.patch128.43 KBWim Leers
#136 interdiff.txt1.04 KBWim Leers
#129 2800873-129.patch128.4 KBWim Leers
#129 interdiff.txt744 bytesWim Leers
#128 2800873-128.patch128.51 KBWim Leers
#128 interdiff.txt6.6 KBWim Leers
#127 2800873-127.patch122.03 KBWim Leers
#127 interdiff.txt6.31 KBWim Leers
#126 2800873-126.patch115.84 KBWim Leers
#126 interdiff.txt4.91 KBWim Leers
#125 2800873-125.patch112.54 KBWim Leers
#125 interdiff.txt25.89 KBWim Leers
#124 2800873-124.patch87.01 KBWim Leers
#124 interdiff.txt3.79 KBWim Leers
#123 2800873-123.patch83.29 KBWim Leers
#123 interdiff.txt9.97 KBWim Leers
#122 2800873-122.patch73.47 KBWim Leers
#122 interdiff.txt4.79 KBWim Leers
#121 2800873-121.patch69.52 KBWim Leers
#121 interdiff.txt26.6 KBWim Leers
#120 2800873-120.patch43.28 KBWim Leers
#120 interdiff.txt6.1 KBWim Leers
#119 2800873-119.patch38.87 KBWim Leers
#119 interdiff.txt10.04 KBWim Leers
#118 2800873-118.patch28.94 KBWim Leers
#118 interdiff.txt4.2 KBWim Leers
#117 2800873-117.patch25.5 KBWim Leers
#117 interdiff.txt3.09 KBWim Leers
#116 2800873-116.patch22.47 KBWim Leers
#116 interdiff.txt4.8 KBWim Leers
#115 2800873-115.patch19.15 KBWim Leers
#115 interdiff.txt4.73 KBWim Leers
#112 2800873-111.patch14.48 KBWim Leers
#112 interdiff.txt1.63 KBWim Leers
#110 2800873-110.patch13.34 KBWim Leers
#110 interdiff.txt3.16 KBWim Leers
#109 2800873-109.patch13.08 KBWim Leers
#109 interdiff.txt3.98 KBWim Leers
#107 2800873-101.patch9.99 KBWim Leers
#101 2800873-101.patch9.99 KBWim Leers
#101 interdiff.txt1.29 KBWim Leers
#100 2800873-100.patch10.78 KBWim Leers
#100 interdiff.txt1.2 KBWim Leers
#95 2800873-95.patch11.93 KBWim Leers
#95 interdiff.txt9.91 KBWim Leers
#88 2800873-88.patch3.23 KBWim Leers
#88 interdiff.txt3.09 KBWim Leers
#87 2800873-87.patch3.14 KBWim Leers
#87 interdiff-3-87.txt1.99 KBWim Leers
#67 interdiff-2800873-67.txt1.69 KBdamiankloip
#67 2800873-67.patch9.93 KBdamiankloip
#62 interdiff-2800873-62.txt1.45 KBdamiankloip
#62 2800873-62.patch9.15 KBdamiankloip
#60 interdiff-2800873-60.txt17.92 KBdamiankloip
#60 2800873-60.patch9.16 KBdamiankloip
#51 2800873-49.patch23.42 KBWim Leers
#49 interdiff.txt3.08 KBWim Leers
#49 2826407-49.patch3.08 KBWim Leers
#46 interdiff.txt3.78 KBlhangea
#46 2800873-mark_xml_as_deprecated_remove_tests_and_services-46.patch22.85 KBlhangea
#44 2800873-mark_xml_as_deprecated_remove_tests_and_services-44.patch22.36 KBlhangea
#41 2800873-mark_xml_as_deprecated_remove_tests_and_services-41.patch20.03 KBlhangea
#32 interdiff.txt8.88 KBlhangea
#32 2800873-mark_xml_as_deprecated_remove_tests_and_services-32.patch26.32 KBlhangea
#30 interdiff.txt10.51 KBlhangea
#30 2800873-mark_xml_as_deprecated_remove_tests_and_services-30.patch17.44 KBlhangea
#19 2800873-19.patch10.86 KByogeshmpawar
#15 2800873-15.patch10.86 KByogeshmpawar
#13 2800873-13.patch11.28 KBWim Leers
#3 2800873-3-tests-only-FAIL.patch1.06 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

damiankloip’s picture

Here 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 :

// ...
elseif (is_numeric($key) || !$this->isElementNameValid($key)) {
    $append = $this->appendNode($parentNode, $data, 'item', $key);
}
neclimdul’s picture

Bah, 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 :(

Wim Leers’s picture

Title: Node serialized in 'xml' format cannot be deserialized » 'xml' format broken: Symfony's XmlEncoder maps a single-item array to a non-array

Retitling accordingly, to indicate this is a generic problem.

Wim Leers’s picture

Also 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 'xml' format is broken in the title.

tstoeckler’s picture

Well, 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?

Wim Leers’s picture

No. This is a generic problem, because all fields have multiple items.

In other words:

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! Their structure is completely different.

tstoeckler’s picture

Ahh, 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.

Wim Leers’s picture

neclimdul’s picture

This 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.

Wim Leers’s picture

I 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:

11:43:57 <WimLeers> https://www.drupal.org/node/2800873#comment-11629699
11:44:04 <Druplicon> https://www.drupal.org/node/2800873 => 'xml' format broken: Symfony's XmlEncoder maps a single-item array to a non-array #2800873: Add XML GET REST test coverage, work around XML encoder quirks => 5 comments, 1 IRC mention
11:44:21 <dawehner> meh, XML
11:45:28 <WimLeers> Yesterday's XML is tomorrow's JSON
11:45:41 <WimLeers> either we support it or we don't
11:45:47 <WimLeers> I'm fine with dropping XML support altogether
11:45:50 <WimLeers> but then we shouldn't pretend it works
11:46:08 <damiankloip> I am still not sure having 2 items in there would fix it like we expect? their xml encoder will try and add everything with a numeric key to the parent node
11:46:57 <WimLeers> that's a good point
11:47:07 <WimLeers> In other words
11:47:08 <WimLeers> numerically indexed arrays simply cannot be represented in XML
11:47:15 <WimLeers> unless you make up arbitrary element names
11:47:33 <WimLeers> Which means you could do an encoding-specific normalization to fix this
11:47:57 <damiankloip> possibly
11:47:57 <WimLeers> but that then violates the whole point of having normalizations and encodings be independent things
11:48:02 <damiankloip> we would have to prefix the keys or something
11:48:04 <WimLeers> yes
11:48:06 <WimLeers> exactly
11:48:12 <damiankloip> but yeah, less than ideal
11:48:12 <WimLeers> well no
11:48:13 <WimLeers> I'd say
11:48:29 <WimLeers> <fieldname><items><item><item></items></fieldname>
11:48:36 <WimLeers> i.e. autoamtically generate <items>
11:48:43 <WimLeers> and then map each numerically keyed thing to an <item>
11:48:57 <WimLeers> _that_ would be in line with what XML expects
11:49:27 <WimLeers> damiankloip: is my conclusion correct? That basically the XML format cannot work at all for any entity right now?
11:49:41 <WimLeers> damiankloip: If so, we should IMO seriously considering dropping XML support
11:49:47 → joachim joined (~joachim@cpc105064-sgyl40-2-0-cust61.18-2.cable.virginm.net)
11:49:56 <WimLeers> damiankloip: it'll take a lot of time to fix this, and it's questionable whether that's worth our time
11:50:07 <damiankloip> I am not sure if trying to add in our own items node would work, as the symfony encoder would just try to do the same thing to that
11:50:07 <WimLeers> It's not even Symfony's fault per se
11:50:15 <damiankloip> it's kind of symfonys fault
11:50:18 <damiankloip> let's blame them
11:50:19 <WimLeers> it is?
11:50:19 <damiankloip> :)
11:50:22 <WimLeers> lol :
11:50:23 <WimLeers> :P
11:50:37 <damiankloip> this affects field items mainly, right?
11:50:39 <WimLeers> yes
11:50:58 <damiankloip> we could maybe just to to have an xml specific denormalization for field items
11:51:19 <damiankloip> and 're-hydrate' it
11:57:44 <damiankloip> hmm, that could get awkward too, would just have to be at the entity level I think
12:03:26 <damiankloip> WimLeers or, we could have our own decoder that fixes the arrays when normalizing to an array
12:03:44 <WimLeers> damiankloip: decoder when normalizing?
12:04:06 <WimLeers> damiankloip: the problem is that you cannot map this 1:1 to XML, at least not without introducing "dummy elements"
12:04:13 <damiankloip> normalizer for fields when using xml to create a custom strcuture
12:04:18 <WimLeers> right
12:04:19 <WimLeers> that works
12:04:29 <WimLeers> but is it worth it?
12:04:32 <WimLeers> This can happen at _any_ level
12:04:44 <WimLeers> it can also happen e.g. for the properties of a certain field
12:04:57 <WimLeers> we can't automatically fix those probably
12:04:57 <damiankloip> Yes, it can, but just some kind of damage limitation
12:05:06 <WimLeers> true
12:05:09 <damiankloip> if a property was an array?
12:05:10 <damiankloip> you mean
12:05:13 <WimLeers> yes
12:05:18 <damiankloip> like ['value' => [some data]]
12:05:21 <WimLeers> yes
12:06:35 <damiankloip> yeah, true
12:07:06 <damiankloip> but we could check all numeric keys. and prefix them like '_#1'
12:07:11 <damiankloip> then change it back or somethin...
12:07:55 <WimLeers> damiankloip: I wonder if we can use attributes (<field item0="value 0" item1="value1">) instead of child elements
12:08:36 <damiankloip> @WimLeers yes, we could do that if we converted the items to '@item0' etc..
12:08:46 <WimLeers> as always in XML/HTML/… "when to use attributes versus child elements"
12:08:52 <damiankloip> then the xml encoder will create attributes for us
12:09:00 <WimLeers> right
12:09:03 <damiankloip> yeah, either way it's kind of a PITA
12:09:22 <WimLeers> it is
12:09:49 <WimLeers> the thing I expected was really <field><item>value 0</item><item>value 1</item></field>
12:10:07 <WimLeers> But there's no way the XML encoder can know that we want it to be called "item"
12:10:11 <WimLeers> because that's not stored in the PHP array
12:10:24 <damiankloip> Yeah
12:10:25 <WimLeers> so, in a way, our PHP data structure is to blame
12:10:31 <damiankloip> that idea would work with my idea above
12:10:35 <damiankloip> prefix all the keys
12:10:36 <WimLeers> yeah
12:10:39 <damiankloip> that are numeric
12:10:44 <damiankloip> maybe that is the simplest way
12:11:02 <WimLeers> well no, the problem is still: "how do you know what label to use for each of the numerically keyed children?"
12:11:10 <WimLeers> that means you still must write a custom normalizer
12:11:15 <WimLeers> which means you can't do it in a generic way
12:11:21 <WimLeers> unless we choose to call them the same thing always
12:11:25 <WimLeers> <item>
12:11:26 <WimLeers> or
12:11:27 <WimLeers> <delta>
12:11:34 <WimLeers> or …
12:11:51 <WimLeers> but it's always going to be "bad" XML
12:11:56 <WimLeers> so… :(
12:12:16 <WimLeers> I think the best path forward is to just deprecate XML support, and leave it completely untouched for those who are using it
12:12:33 <WimLeers> and potentially to just remove it, but only in 8.3
12:22:26 <damiankloip> well, we could generically create our own keys and rely on them I tihnk
12:22:31 <damiankloip> but removing it is easier
Wim Leers’s picture

Status: Active » Needs review
FileSize
11.28 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 13: 2800873-13.patch, failed testing.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
10.86 KB

I have rerolled the patch to apply with 8.2.x branch.

neclimdul’s picture

Version: 8.2.x-dev » 8.3.x-dev

This could be a disruptive change. 8.2 might be a bit much since its in RC so lets target 8.3 for now.

Wim Leers’s picture

Sure, but:

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.

I'm fine with leaving it in its current status during 8.2.x.

Status: Needs review » Needs work

The last submitted patch, 15: 2800873-15.patch, failed testing.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
10.86 KB

Same patch testing against 8.3.x branch.

Status: Needs review » Needs work

The last submitted patch, 19: 2800873-19.patch, failed testing.

Wim Leers’s picture

#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.

Wim Leers’s picture

Issue tags: +php-novice
vprocessor’s picture

Assigned: Unassigned » vprocessor
Wim Leers’s picture

Woot! Thanks vprocessor! I'll provide reviews!

damiankloip’s picture

Yes, I am happy with that approach also. Custom XML formats should definitely be supported.

lhangea’s picture

vprocessor please let me know if you are still working on it. I can help here if needed.

Wim Leers’s picture

Assigned: vprocessor » Unassigned

@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 :)

lhangea’s picture

tomorrow I will be on my way to IronCamp so maybe the next day I will be able to work on it :)

Wim Leers’s picture

Cool! :)

lhangea’s picture

Let'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.

Status: Needs review » Needs work
lhangea’s picture

vprocessor’s picture

Sorry guys, was too busy :-(

Status: Needs review » Needs work
lhangea’s picture

Status: Needs work » Needs review
lhangea’s picture

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

@lhangea: Looking good! :) Thanks!
@vprocessor: No worries! We all get unexpectedly busy :)

  1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -292,8 +255,7 @@ public function testRestRenderCaching() {
    -    // Request the page, once in XML and once in JSON to ensure that the caching
    -    // varies by it.
    

    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.

  2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -338,80 +290,6 @@ public function testRestRenderCaching() {
    -  public function testResponseFormatConfiguration() {
    

    This should not be deleted altogether. The relevant portions should be kept.

  3. +++ b/core/modules/serialization/src/Encoder/XmlEncoder.php
    @@ -11,6 +11,8 @@
    + * @deprecated
    

    This needs to be more specific.

     * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0. This exists solely for BC, for classes extending this.
    
  4. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -85,98 +84,85 @@ protected function loginRequest($name, $pass, $format = 'json') {
    -    foreach ([FALSE, TRUE] as $serialization_enabled_option) {
    -      if ($serialization_enabled_option) {
    -        /** @var \Drupal\Core\Extension\ModuleInstaller $module_installer */
    

    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 the hal 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 :)

Wim Leers’s picture

Wim Leers’s picture

lhangea’s picture

Status: Needs work » Needs review

Thanks 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.

lhangea’s picture

Status: Needs review » Needs work

lhangea’s picture

All 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.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Novice, -php-novice
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -809,15 +809,15 @@ public function testPatch() {
     // DX: 415 when request body in existing but not allowed format.

This 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 the rest_test module. Let's let that module add a new foobar format (which doesn't need to actually exist, it just needs to be registered, so that these tests can work), and use that instead of xml in EntityResourceTestBase. Problem solved!

Once that's done, this is ready! Thanks for pushing this forward :)

lhangea’s picture

I 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: ...in ContentTypeHeaderMatcher.
But let's try it anyway here, maybe something's wrong with my local.

Status: Needs review » Needs work
damiankloip’s picture

+++ b/core/modules/rest/tests/modules/rest_test/src/EventSubscriber/FormatProvider.php
@@ -0,0 +1,31 @@
+    $event->getRequest()->setFormat('foobar', ['text/foobar']);
...
+    $events[KernelEvents::REQUEST][] = ['onKernelRequest'];

I think this is registered too late. IIRC you will need to register this in a ServiceProvider instead and add code this in there:

    if ($container->has('http_middleware.negotiation')) {
      $negotiation = $container->getDefinition('http_middleware.negotiation');
      $negotiation->addMethodCall('registerFormat', ['foobar', ['text/foobar']]);
    }

EDIT: Yes, just checked. See HalServiceProvider for a good example :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
3.08 KB

#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.

Status: Needs review » Needs work

The last submitted patch, 49: 2826407-49.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
23.42 KB

Oops. This is the right patch.

Status: Needs review » Needs work

The last submitted patch, 51: 2800873-49.patch, failed testing.

damiankloip’s picture

Ah. Yes - I thought the patch already had that part for some reason. Nice!

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

This 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 :

$this->assertFalse($encoder->supportsEncoding('xml'));

I will do the CR today.

damiankloip’s picture

Wim Leers’s picture

Issue tags: -Needs change record

YAY!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'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?

  1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -575,25 +522,6 @@ public function testLivePreview() {
    -          'xml' => 'xml',
    

    There's no upgrade path for existing views using xml. What happens to them?

  2. +++ /dev/null
    @@ -1,79 +0,0 @@
    -class XmlEncoderTest extends UnitTestCase {
    

    Why wouldn't we leave this test coverage here if we are deprecating the class - we still need to test it.

damiankloip’s picture

+++ b/core/modules/serialization/src/Encoder/XmlEncoder.php
@@ -11,6 +11,9 @@
+ * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0. This
+ *   exists solely for BC, for classes extending this.

Hmm, 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.

Wim Leers’s picture

So 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 use xml, without allowing the xml format? Keeping the encoding is okay, but the xml format is not. Remember: format = normalization + encoding.

+++ b/core/modules/serialization/serialization.services.yml
@@ -49,10 +49,6 @@ services:
-  serializer.encoder.xml:
-    class: Drupal\serialization\Encoder\XmlEncoder
-    tags:
-      - { name: encoder, format: xml }

This must absolutely be removed.

So that leaves us no choice but to hardcode XML support in \Drupal\rest\Plugin\views\style\Serializer somehow.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
9.16 KB
17.92 KB

Not 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:

  1. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -199,6 +199,12 @@ public function calculateDependencies() {
    +    $formats[] = 'xml';
    

    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.

  2. +++ b/core/modules/serialization/serialization.services.yml
    @@ -52,7 +52,10 @@ services:
    +    - { name: encoder }
    

    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.

  3. +++ b/core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php
    @@ -42,6 +42,10 @@ public function process(ContainerBuilder $container) {
    +      if (!isset($attributes[0]['format'])) {
    

    Small change here to allow no format.

Wim Leers’s picture

Not sure what you mean about "format = normalization + encoding." ?

I 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 the application/hal+json MIME type, which uses the hal normalization and the json encoding. Combine that normalization and that encoding and you end up with the HAL+JSON format.

I was under the perhaps mistaken impression that Views RestExport displays only use the xml encoding, not the xml format.

Well, no, I can't be mistaken I think, because \Drupal\rest\Plugin\views\style\Serializer::render looks like this:

  public function render() {
    $rows = array();
    // If the Data Entity row plugin is used, this will be an array of entities
    // which will pass through Serializer to one of the registered Normalizers,
    // which will transform it to arrays/scalars. If the Data field row plugin
    // is used, $rows will not contain objects and will pass directly to the
    // Encoder.
    foreach ($this->view->result as $row_index => $row) {
      $this->view->row_index = $row_index;
      $rows[] = $this->view->rowPlugin->render($row);
    }
    unset($this->view->row_index);

    // Get the content type configured in the display or fallback to the
    // default.
    if ((empty($this->view->live_preview))) {
      $content_type = $this->displayHandler->getContentType();
    }
    else {
      $content_type = !empty($this->options['formats']) ? reset($this->options['formats']) : 'json';
    }
    return $this->serializer->serialize($rows, $content_type, ['views_style_plugin' => $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 the xml encoder, which would hence be able to avoid those limitations/bugs.

So yes, in a nutshell, we need to keep the XmlEncoder with the 'name' tag (encoder) but not the 'format' attribute.

Sounds good!


  1. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -199,6 +199,12 @@ public function calculateDependencies() {
    +    // and will be removed in Drupal 9. However, it must continue to work with
    +    // existing REST Exports.
    

    I think this also allows new REST Exports to be created. Should the comment be updated to reflect that?

  2. +++ b/core/modules/serialization/serialization.services.yml
    @@ -52,7 +52,10 @@ services:
    +    # The XML 'format' attribute has been removed as XML encoding is deprecated
    

    s/deprecated/unsupported/

  3. +++ b/core/modules/serialization/serialization.services.yml
    @@ -52,7 +52,10 @@ services:
    +    # in Drupal core.
    

    Should mention 8.3.x specifically.

damiankloip’s picture

Thanks, 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.

Wim Leers’s picture

If they are not using multi value field for example, that data is probably usable.

Exactly. 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 :)

damiankloip’s picture

That sounds reasonable :) I'll add a comment to that effect.

Wim Leers’s picture

IMHO back to RTBC once that comment is added.

Wim Leers’s picture

Status: Needs review » Needs work
damiankloip’s picture

Status: Needs work » Needs review
FileSize
9.93 KB
1.69 KB

Here 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?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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?

@damiankloip and I discussed that on IRC yesterday:

11:56:03 <damiankloip> WimLeers Also, thinking about https://www.drupal.org/node/2800873 if we remove the format that's registered does that break current REST configuration? if an endpoint is using xml as an allowed format.
11:56:06 <Druplicon> https://www.drupal.org/node/2800873 => 'xml' format broken: Symfony's XmlEncoder maps a single-item array to a non-array #2800873: Add XML GET REST test coverage, work around XML encoder quirks => 66 comments, 5 IRC mentions
12:33:40 <WimLeers> damiankloip: check that proper nit, there's just one more proper nit :P
12:34:07 <WimLeers> damiankloip: yes, it'd break current REST configuration. But I'm willing to bet that not a single site out there is using xml, precisely because it doesn't work.
damiankloip’s picture

ok, works for me. Let's see what the 8.x maintainers think I guess :)

Wim Leers’s picture

Yep, exactly!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 67: 2800873-67.patch, failed testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

I think this is ok again (CI fail).

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
@@ -98,7 +97,7 @@ public function testLogin() {
-        $formats = ['json', 'xml', 'hal_json'];
+        $formats = ['json', 'hal_json'];

This 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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I'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 and xml (because that would mean that that test would not need to install the hal module too). Only in the past 2 weeks did we add hal_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 using xml, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs release manager review

In other words: if I didn't insist on better test coverage back then, you wouldn't have raised this concern.

Well 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.

Wim Leers’s picture

That'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 Closed (works as designed) 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 the xml format, that's unfortunately just not possible.

dawehner’s picture

I'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?

Wim Leers’s picture

GETting any entity with a multi-value field that actually has multiple values is broken.

dawehner’s picture

GETting any entity with a multi-value field that actually has multiple values is broken.

I try to understand why this is broken ...

The resulting XML looks like the following:

<?xml version="1.0"?>
<response>
    <nid>
        <value>2</value>
    </nid>
    <field_multi>
        <value>1</value>
    </field_multi>
    <field_multi>
        <value>2</value>
    </field_multi>
    <field_multi>
        <value>3</value>
    </field_multi>
    <field_multi>
        <value>4</value>
    </field_multi>
    <field_image/>
    <field_tags/>
</response>

This for itself is a perfectly valid XML, which you can process:

$file = file_get_contents('test.xml');
$xml = simplexml_load_string($file);
foreach ($xml->xpath('field_multi') as $field) {  print_r((string) $field->value); }

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?

damiankloip’s picture

Right, 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 :)

dawehner’s picture

Right, so that main problem is that it doesn't actual represent the structure of the data we have.

I 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.

Wim Leers’s picture

But 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.

+++ b/core/modules/serialization/serialization.services.yml
@@ -52,7 +52,10 @@ services:
   serializer.encoder.xml:
     class: Drupal\serialization\Encoder\XmlEncoder
     tags:
-      - { name: encoder, format: xml }
+    # The XML 'format' attribute has been removed as XML encoding is unsupported
+    # in Drupal 8.3.x.
+    # @see \Drupal\serialization\Encoder\XmlEncoder
+    - { name: encoder }

But also this! This means you can still create custom XML-based formats. without having to reimplement everything.

+++ b/core/modules/serialization/src/Encoder/XmlEncoder.php
@@ -11,6 +11,10 @@
+ * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0. This
+ *   exists solely for BC, for classes extending this and existing usage that is
+ *   already configured to use or allowed the XML format.
  */
 class XmlEncoder implements EncoderInterface, DecoderInterface {
dawehner’s picture

Sorry 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?

damiankloip’s picture

So 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?

dawehner’s picture

. Maybe we are tied here to just marking the classes as deprecated?

Well, for me the only sane steps from a BC point of view are

  1. Make the classes deprecated
  2. Disable it by default but enable it via some configuration on an update

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Let'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.


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?

Yes, this continues to work.

+++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
@@ -199,6 +204,12 @@ public function calculateDependencies() {
+    // Add XML to the list of formats. It has been deprecated in Drupal 8.3.x
+    // and will be removed in Drupal 9. However, it must continue to work for
+    // REST Exports.
+    $formats[] = 'xml';

This means REST views that use XML continue to work.

+++ b/core/modules/serialization/serialization.services.yml
@@ -52,7 +52,10 @@ services:
   serializer.encoder.xml:
     class: Drupal\serialization\Encoder\XmlEncoder
     tags:
-      - { name: encoder, format: xml }
+    # The XML 'format' attribute has been removed as XML encoding is unsupported
+    # in Drupal 8.3.x.
+    # @see \Drupal\serialization\Encoder\XmlEncoder
+    - { name: encoder }

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

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.

It is possible people are using this, but if they are, they can only be using it for GET or DELETE, not for POST or PATCH.

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 in state. We then have a ServiceModifierInterface implementation that reads this value from state and removes the format: xml portion of the serializer.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 the Node entity type, in the xml format.

Wim Leers’s picture

FileSize
3.09 KB
3.23 KB

Improving 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 and PATCH 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 IS
  • DELETE works because there's no normalization to send nor receive

The last submitted patch, 87: 2800873-87.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 88: 2800873-88.patch, failed testing.

Wim Leers’s picture

#88 confirmed exactly in the failed test output:

1 = GET
2 = POST (400 because bundle is missing)
3 = PATCH (400 because bundle is missing)

There were 3 failures:

1) Drupal\Tests\rest\Functional\EntityResource\Node\NodeXmlAnonTest::testGet
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'nid' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'uuid' => Array (
-        0 => Array (...)
+        'value' => '5b50319f-cea2-447e-bb78-0cd3adeb4ecd'
     )
     'vid' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'langcode' => Array (
-        0 => Array (...)
+        'value' => 'en'
     )
     'type' => Array (
-        0 => Array (...)
+        'target_id' => 'camelids'
+        'target_type' => 'node_type'
+        'target_uuid' => 'f972fd90-13b2-4d92-9c33-ffae0df7719d'
     )
     'title' => Array (
-        0 => Array (...)
+        'value' => 'Llama'
     )
     'status' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'created' => Array (
-        0 => Array (...)
+        'value' => '123456789'
     )
     'changed' => Array (
-        0 => Array (...)
+        'value' => '123456789'
     )
     'promote' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'sticky' => Array (
-        0 => Array (...)
+        'value' => '0'
     )
     'revision_timestamp' => Array (
-        0 => Array (...)
+        'value' => '123456789'
     )
     'revision_translation_affected' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'default_langcode' => Array (
-        0 => Array (...)
+        'value' => '1'
     )
     'uid' => Array (
-        0 => Array (...)
+        'target_id' => '0'
+        'target_type' => 'user'
+        'target_uuid' => '9c2d99b4-4d37-4df7-b707-6308920c4e83'
+        'url' => '/checkout/user/0'
     )
     'revision_uid' => Array (
-        0 => Array (...)
+        'target_id' => '0'
+        'target_type' => 'user'
+        'target_uuid' => '9c2d99b4-4d37-4df7-b707-6308920c4e83'
+        'url' => '/checkout/user/0'
     )
-    'revision_log' => Array ()
+    'revision_log' => ''
 )

/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:1343
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:379

2) Drupal\Tests\rest\Functional\EntityResource\Node\NodeXmlAnonTest::testPost
Failed asserting that 400 is identical to 403.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:301
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:325
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:575

3) Drupal\Tests\rest\Functional\EntityResource\Node\NodeXmlAnonTest::testPatch
Failed asserting that 400 is identical to 403.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:301
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:325
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:766
dawehner’s picture

That would mean only sites that have used the xml format would still be able to use it. This would reduce the maintenance burden.

Well, this causes issues with installation profiles, doesn't it?

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes
Status: Needs work » Active
Issue tags: -Needs release manager review

IS 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 Needs release manager review tag.

dawehner’s picture

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 :(

No 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.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
9.91 KB
11.93 KB

Here'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!

Wim Leers’s picture

Title: 'xml' format broken: Symfony's XmlEncoder maps a single-item array to a non-array » Add XML REST test coverage, work around XML encoder quirks
Issue tags: +API-First Initiative

Better title to reflect #95's direction.

Wim Leers’s picture

Note: we still need to add test coverage for all other entity types!

Status: Needs review » Needs work

The last submitted patch, 95: 2800873-95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

I 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…

Wim Leers’s picture

Title: Add XML REST test coverage, work around XML encoder quirks » Add XML GET REST test coverage, work around XML encoder quirks
Status: Needs work » Needs review
FileSize
1.2 KB
10.78 KB

This 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.

Wim Leers’s picture

FileSize
1.29 KB
9.99 KB
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -766,7 +768,8 @@ public function testPost() {
-    $this->assertResourceErrorResponse(400, 'Syntax error', $response);
+    // @todo this is apparently JSON-specific
+//    $this->assertResourceErrorResponse(400, 'Syntax error', $response);

@@ -998,7 +1001,8 @@ public function testPatch() {
-    $this->assertResourceErrorResponse(400, 'Syntax error', $response);
+    // @todo this is apparently JSON-specific
+//    $this->assertResourceErrorResponse(400, 'Syntax error', $response);

These 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.

The last submitted patch, 100: 2800873-100.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 101: 2800873-101.patch, failed testing. View results

dawehner’s picture

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -464,8 +464,10 @@ public function testGet() {
    +    if (static::$format !== 'xml') {
    

    We should document this if potentially?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
    @@ -0,0 +1,111 @@
    +      if (!empty($normalization[$field_name])) {
    +        $normalization[$field_name] = $normalization[$field_name][0];
    +      }
    

    It would be nice to maybe have a comment about this one. It feels like this makes it impossible to have multivalue fields, right?

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
    @@ -0,0 +1,111 @@
    +  public function testPost() {
    +    // Deserialization of the XML format is not supported.
    +    $this->markTestSkipped();
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function testPatch() {
    +    // Deserialization of the XML format is not supported.
    +    $this->markTestSkipped();
    +  }
    

    Nice!

Wim Leers’s picture

Issue tags: +Needs tests
  1. Of course!
  2. Right, I forgot to expand test coverage in a very essential way: a multi-value field. I was planning to do that, but forgot after it already took quite a lot of time to deal with the many edge cases besides multi-value fields. My bad. Very glad you called that out though.
  3. :)

Also:

Note: we still need to add test coverage for all other entity types!

But at least we have a starting point now!

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
9.99 KB

Let's get this going again. Let's first reupload #101, because AFAICT it's passing locally now, without any further changes.

Status: Needs review » Needs work

The last submitted patch, 107: 2800873-101.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
13.08 KB

#104

  1. Done.
  2. Multi-field value test coverage added

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!

Wim Leers’s picture

FileSize
3.16 KB
13.34 KB

I noticed a bug in #109, which will cause every config entity test to fail.

The last submitted patch, 109: 2800873-109.patch, failed testing. View results

Wim Leers’s picture

FileSize
1.63 KB
14.48 KB

So 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 a Vary 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!

The last submitted patch, 110: 2800873-110.patch, failed testing. View results

Wim Leers’s picture

Yay! Now: expand this to all other entity types!

Wim Leers’s picture

FileSize
4.73 KB
19.15 KB

Adding Comment (the alphabetically first content entity type).

Zero additional changes were necessary — I just had to add the necessary test classes :)

Wim Leers’s picture

FileSize
4.8 KB
22.47 KB

Adding Feed (the alphabetically second content entity type).

I had to update XmlEntityNormalizationQuirksTrait::applyXmlFieldDecodingQuirks() to special-case the ListIntegerItem and TimestampItem field types' normalizations XML decoding quirks.

Wim Leers’s picture

FileSize
3.09 KB
25.5 KB

Adding Item (the alphabetically third content entity type).

Zero additional changes were necessary — I just had to add the necessary test classes :)

Wim Leers’s picture

FileSize
4.2 KB
28.94 KB

Adding 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.

Wim Leers’s picture

FileSize
10.04 KB
38.87 KB

Adding Shortcut, Term and User. 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 :)

Wim Leers’s picture

FileSize
6.1 KB
43.28 KB

Adding Action (the alphabetically first config entity type).

I had to add XmlEntityNormalizationQuirksTrait::applyXmlConfigEntityDecodingQuirks().

Wim Leers’s picture

FileSize
26.6 KB
69.52 KB

Added 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 :)

Wim Leers’s picture

FileSize
4.79 KB
73.47 KB

Added 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:

                } elseif (is_array($data) && false === is_numeric($key)) {
                    // Is this array fully numeric keys?
                    if (ctype_digit(implode('', array_keys($data)))) {
                        /*
                         * Create nodes to append to $parentNode based on the $key of this array
                         * Produces <xml><item>0</item><item>1</item></xml>
                         * From array("item" => array(0,1));.
                         */
                        foreach ($data as $subData) {
                            $append = $this->appendNode($parentNode, $subData, $key);
                        }
                    } else {
                        $append = $this->appendNode($parentNode, $data, $key);
                    }
                } elseif (is_numeric($key) || !$this->isElementNameValid($key)) {
                    $append = $this->appendNode($parentNode, $data, 'item', $key);
                }

This means that you end up not with

array(1) {
  [0]=>
  array(5) {
    [0]=>
    array(2) {
      ["items"]=>
      array(2) {
        [0]=>
        string(4) "Bold"
        [1]=>
        string(6) "Italic"
      }
      ["name"]=>
      string(10) "Formatting"
    }
    [1]=>
    array(2) {
      ["items"]=>
      array(2) {
        [0]=>
        string(10) "DrupalLink"
        [1]=>
        string(12) "DrupalUnlink"
      }
      ["name"]=>
      string(5) "Links"
    }
    [2]=>
    array(2) {
      ["items"]=>
      array(2) {
        [0]=>
        string(12) "BulletedList"
        [1]=>
        string(12) "NumberedList"
      }
      ["name"]=>
      string(5) "Lists"
    }
    [3]=>
    array(2) {
      ["items"]=>
      array(2) {
        [0]=>
        string(10) "Blockquote"
        [1]=>
        string(11) "DrupalImage"
      }
      ["name"]=>
      string(5) "Media"
    }
    [4]=>
    array(2) {
      ["items"]=>
      array(1) {
        [0]=>
        string(6) "Source"
      }
      ["name"]=>
      string(5) "Tools"
    }
  }
}

but with:

array(5) {
  [0]=>
  array(3) {
    ["@key"]=>
    int(0)
    ["items"]=>
    array(2) {
      [0]=>
      string(4) "Bold"
      [1]=>
      string(6) "Italic"
    }
    ["name"]=>
    string(10) "Formatting"
  }
  [1]=>
  array(3) {
    ["@key"]=>
    int(1)
    ["items"]=>
    array(2) {
      [0]=>
      string(10) "DrupalLink"
      [1]=>
      string(12) "DrupalUnlink"
    }
    ["name"]=>
    string(5) "Links"
  }
  [2]=>
  array(3) {
    ["@key"]=>
    int(2)
    ["items"]=>
    array(2) {
      [0]=>
      string(12) "BulletedList"
      [1]=>
      string(12) "NumberedList"
    }
    ["name"]=>
    string(5) "Lists"
  }
  [3]=>
  array(3) {
    ["@key"]=>
    int(3)
    ["items"]=>
    array(2) {
      [0]=>
      string(10) "Blockquote"
      [1]=>
      string(11) "DrupalImage"
    }
    ["name"]=>
    string(5) "Media"
  }
  [4]=>
  array(3) {
    ["@key"]=>
    int(4)
    ["items"]=>
    string(6) "Source"
    ["name"]=>
    string(5) "Tools"
  }
}

(Note one level less in the array, and the use of @key.)

Wim Leers’s picture

FileSize
9.97 KB
83.29 KB

Added FieldConfig + FieldStorageConfig + FilterFormat.

Zero additional changes were necessary — I just had to add the necessary test classes :)

Wim Leers’s picture

Added 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:

Fstring(2801) "The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">DOMException</em>: Invalid Character Error in <em class="placeholder">DOMDocument-&gt;createElement()</em> (line <em class="placeholder">452</em> of <em class="placeholder">vendor/symfony/serializer/Encoder/XmlEncoder.php</em>). <pre class="backtrace">Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;appendNode(Object, Array, &#039;574d9c11-33ec-4fa7-9a6b-6783600c9cfa&#039;) (Line: 408)
Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;buildXml(Object, Array) (Line: 490)
Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;selectNodeType(Object, Array) (Line: 456)
Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;appendNode(Object, Array, &#039;effects&#039;) (Line: 408)
Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;buildXml(Object, Array, &#039;response&#039;) (Line: 65)
Symfony\Component\Serializer\Encoder\XmlEncoder-&gt;encode(Array, &#039;xml&#039;, Array) (Line: 60)
Drupal\serialization\Encoder\XmlEncoder-&gt;encode(Array, &#039;xml&#039;, Array) (Line: 38)
Symfony\Component\Serializer\Encoder\ChainEncoder-&gt;encode(Array, &#039;xml&#039;, Array) (Line: 233)
Symfony\Component\Serializer\Serializer-&gt;encode(Array, &#039;xml&#039;, Array) (Line: 118)
Symfony\Component\Serializer\Serializer-&gt;serialize(Array, &#039;xml&#039;) (Line: 156)
Drupal\rest\EventSubscriber\ResourceResponseSubscriber-&gt;Drupal\rest\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 157)
Drupal\rest\EventSubscriber\ResourceResponseSubscriber-&gt;renderResponseBody(Object, Object, Object, &#039;xml&#039;) (Line: 74)
Drupal\rest\EventSubscriber\ResourceResponseSubscriber-&gt;onResponse(Object, &#039;kernel.response&#039;, Object) (Line: 108)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(&#039;kernel.response&#039;, Object) (Line: 193)
Symfony\Component\HttpKernel\HttpKernel-&gt;filterResponse(Object, Object, 1) (Line: 175)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache-&gt;pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
</pre>"

The root cause:

        $this->effectUuid => [
          'uuid' => $this->effectUuid,
          'id' => 'image_scale_and_crop',
          'weight' => 0,
          'data' => [
            'width' => 120,
            'height' => 121,
          ],
        ],

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.

Wim Leers’s picture

FileSize
25.89 KB
112.54 KB

Added Menu + NodeType + RdfMapping + ResponsiveImageStyle + RestResourceConfig + Role + SearchPage + ShortcutSet.

Zero additional changes were necessary — I just had to add the necessary test classes :)

Wim Leers’s picture

FileSize
4.91 KB
115.84 KB

Added Tour.

This time there is another XML decoding quirk that wasn't yet taken into account.

Wim Leers’s picture

FileSize
6.31 KB
122.03 KB

Added 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.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
6.6 KB
128.51 KB

Added EntityTest + EntityTestLabel.

Test coverage complete!

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
FileSize
744 bytes
128.4 KB

Updated IS. Reviewed the patch myself:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedXmlBasicAuthTest.php
@@ -0,0 +1,40 @@
+  public function testGet() {
+    return parent::testGet(); // TODO: Change the autogenerated stub
+  }

This is a debug leftover that should be deleted.

Wim Leers’s picture

Version: 8.5.x-dev » 8.4.x-dev
Category: Bug report » Task

Since 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.

dawehner’s picture

Note: I'm using emoji based code review, see https://gist.github.com/pfleidi/4422a5cac5b04550f714f1f886d2feea

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentXmlBasicAuthTest.php
    @@ -0,0 +1,52 @@
    +  public function testPostDxWithoutCriticalBaseFields() {
    +    // Deserialization of the XML format is not supported.
    +    $this->markTestSkipped();
    ...
    +  public function testPostSkipCommentApproval() {
    +    // Deserialization of the XML format is not supported.
    +    $this->markTestSkipped();
    +  }
    

    🤔 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 ?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Editor/EditorXmlCookieTest.php
    index 72589bd..eac468f 100644
    --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    
    --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    

    🤡 Do we actually test updating of a multivalue field? I guess this is out of scope of this issue.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
    @@ -0,0 +1,156 @@
    +    if ($this->entity instanceof FieldableEntityInterface) {
    +      $normalization = $this->applyXmlFieldDecodingQuirks($default_normalization);
    +    }
    +    else {
    +      $normalization = $this->applyXmlConfigEntityDecodingQuirks($default_normalization);
    +    }
    ...
    +    if (!$this->entity instanceof FieldableEntityInterface) {
    +      throw new \LogicException('This trait should only be used for fieldable entity types.');
    +    }
    ...
    +    if (!$this->entity instanceof ConfigEntityInterface) {
    +      throw new \LogicException('This trait should only be used for config entity types.');
    +    }
    +
    

    ❓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?

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
    @@ -0,0 +1,156 @@
    +   *
    +   * The XML encoding:
    +   * - loses type data (int and bool become string)
    

    🤡 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.

The last submitted patch, 127: 2800873-127.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 126: 2800873-126.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 128: 2800873-128.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 129: 2800873-129.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
128.43 KB

#126 fixed something for Tour, but caused a regression for ResponsiveImageStyle. Fixing that.

Wim Leers’s picture

FileSize
1.58 KB
128.25 KB

Fixed coding standards violations.

Wim Leers’s picture

#131: 🙏❤️

  1. We already have
    /**
       * {@inheritdoc}
       */
      public function testPost() {
        // Deserialization of the XML format is not supported.
        $this->markTestSkipped();
      }
    
      /**
       * {@inheritdoc}
       */
      public function testPatch() {
        // Deserialization of the XML format is not supported.
        $this->markTestSkipped();
      }
    

    in XmlEntityNormalizationQuirksTrait. The two overrides you are quoting are UserResourceTestBase-specific tests.

  2. Yes, out of scope. But very good point 👏. Issue created: #2905686: Expand EntityResourceTestBase to test PATCHing multi-value fields: both adding and removing items
  3. Right, we don't need the exceptions anymore. Done. 🎉
  4. Uhm yeah, that's hugely out of scope — not in the least because it'd mean changing the XML serialization of entities 😱 The whole point of this issue is to have test coverage for XML serializations of entities, to prevent regressions in the future. We shipped with Symfony's rather particular XML encoding process, so that's what we now have to support for the foreseeable future.
Wim Leers’s picture

Also, this current patch means the draft CR at https://www.drupal.org/node/2835098 could be deleted :)

The last submitted patch, 136: 2800873-136.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Uhm yeah, that's hugely out of scope

Yeah see my used emoji :)

in XmlEntityNormalizationQuirksTrait. The two overrides you are quoting are UserResourceTestBase-specific tests.

Oh I see, this makes sense :)

damiankloip’s picture

+1 on this RTBC, super thorough - looks great.

+++ b/core/modules/rest/tests/src/Functional/XmlNormalizationQuirksTrait.php
@@ -0,0 +1,63 @@
+        // Restructure multiple-item arrays inside a single-item numeric array.
+        // @see \Symfony\Component\Serializer\Encoder\XmlEncoder::buildXml()

Alas, this is one of our big issues with XML :/

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 138: 2800873-138.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Random fail in SettingsTrayBlockFormTest (JS test).

Retesting.

Wim Leers’s picture

Since #138 was RTBC since August 31, the following issues have landed, which added more test coverage that should also get XML test coverage:

  1. ContactForm: #2843778: EntityResource: Provide comprehensive test coverage for ContactForm entity
  2. EntityTestBundle: #2843776: EntityResource: Provide comprehensive test coverage for EntityTestBundle entity
  3. BlockContent: #2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity
  4. Media + MediaType: #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types

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 :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 145: 2800873-145.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Reviewed & tested by the community

#2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity only landed for 8.5. Which means this is now an 8.5-only issue.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

Hey all, can we get an issue summary update - it still lists 4 possible solutions?

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

The IS is up-to-date, actually. The first 3 are struck through with <del>, the fourth proposal is not. Clarified this.

Wim Leers’s picture

Wim Leers’s picture

FileSize
20.99 KB
161.87 KB

Since #145, the following landed:

  1. #2843780: EntityResource: Provide comprehensive test coverage for EntityFormMode entity
  2. #2843781: EntityResource: Provide comprehensive test coverage for EntityViewMode entity
  3. #2843755: EntityResource: Provide comprehensive test coverage for Message entity
  4. #2843764: EntityResource: Provide comprehensive test coverage for EntityFormDisplay entity
  5. #2843765: EntityResource: Provide comprehensive test coverage for EntityViewDisplay entity

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:

  1. conflicted slightly with #2843755: EntityResource: Provide comprehensive test coverage for Message entity, in ::setUp()
  2. needed to be updated to accommodate for the testPatchPath() method that was added to NodeResourceTestBase and TermResourceTestBase

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.

dawehner’s picture

Just a drive by comment. The test coverage looks great for me!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -557,6 +603,18 @@ public function testGet() {
+      if ($this->entity instanceof FieldableEntityInterface) {
+        $expected += [
+          'field_rest_test_multivalue' => [
+            0 => [
+              'value' => 'One',
+            ],

An idea for a future follow up: Ensure that adding/removing items from the list works.

Wim Leers’s picture

You 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. 🙂

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2800873-151.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

My brain is porous, sorry for that :)

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

This 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!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2800873-151.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Hm, this started segfaulting apparently. Retesting.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2800873-151.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
161.94 KB

#2901562: Fix 'Squiz.WhiteSpace.SuperfluousWhitespace' coding standard landed yesterday and caused a conflict. Easy conflict resolution.

larowlan’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -468,11 +502,11 @@ public function testGet() {
+    // may be to GET requests - depending on web server configuration. They are

be => 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?

Wim Leers’s picture

FileSize
1.32 KB
161.95 KB

Just for my understanding - we're only adding tests here?

Correct. We're adding <EntityType>Xml<AuthProvider>Test classes and we're expanding the base test coverage to explicitly test multi-value fields.

I was expecting to see something changed outside tests - but its just for tests?

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.


be => be added? can be fixed on commit

Fixed.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.33 KB
165.21 KB

#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 and XmlCookieTest). But then I realized that the error generated by EntityResourceRestTestCoverageTest 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:

'workflow: Workflow (Drupal\workflows\Entity\Workflow), default normalization (expected tests: WorkflowXmlAnonTest, WorkflowXmlBasicAuthTest, WorkflowXmlCookieTest)'

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!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.03 KB
168.17 KB

And this then adds the trivial WorkflowXml*Test test coverage, which makes the patch green again.

The last submitted patch, 163: 2800873-163.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

FileSize
1.74 KB
168.12 KB

#163 and #164 yielded exactly the expected results :)

This is also adding two coding standards violations:

$ composer phpcs  core/modules/rest
> phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- 'core/modules/rest'

FILE: .../Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...odules/rest/tests/src/Functional/XmlNormalizationQuirksTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 42 | ERROR | [x] There should be no white space before a closing "]"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 2.2 secs; Memory: 24Mb

Script phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- handling the phpcs event returned with error code 1
wim.leers at acquia in ~/Work/d8 on 8.5.x*

Fixed those.

Interestingly, PHPStorm failed to mark that unused use statement as unused. PHPStorm--; PHPCS++.

larowlan’s picture

Adding review credits

  • larowlan committed c53b64f on 8.5.x
    Issue #2800873 by Wim Leers, lhangea, damiankloip, Yogesh Pawar,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Fixed!

Committed as c53b64f and pushed to 8.5.x.

:tada:

Wim Leers’s picture

Issue summary: View changes

OMG 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:

  1. Unpostponed follow-ups: #2905655-2: XML encoder does not support UUIDs as keys: makes ImageStyle config entity XML serialization crash and #2905686-3: Expand EntityResourceTestBase to test PATCHing multi-value fields: both adding and removing items.
  2. This also means #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler should now add XML test coverage.
  3. Another plan issue is now less blocked: #2910883-8: Move all entity type REST tests to the providing modules went down from being blocked on 3, to only being blocked on 2!
  4. Last but not least, this means the list of top priorities at #2905563: REST: top priorities for Drupal 8.5.x shrank a bit! This has been a top priority since 8.3: #2794263: REST: top priorities for Drupal 8.3.x.
Wim Leers’s picture

Created a new follow-up, for making this problem go away in D9: #2926034: Deprecate 'xml' serialization format in Drupal 10.

Status: Fixed » Closed (fixed)

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