This is a follow-up for #2113345, see #2113345-129: Define a mechanism for custom link relationships:

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -340,4 +356,35 @@ public function calculateDependencies() {
+        $relationship = $relation_name;
+        if (!empty($definition['uri'])) {
+          $relationship = $definition['uri'];
+        }

Would be nice to use the interface here instead of the definition array. But that requires instantiating the plugin, which maybe has a performance cost that needs to be looked at. Which is why I'm ok punting it to a follow-up.

CommentFileSizeAuthor
#15 2848927-15.patch3.32 KBWim Leers
#15 interdiff.txt1015 bytesWim Leers
#7 2848927-7.patch3.33 KBWim Leers
#4 interdiff.txt1.56 KBWim Leers
#4 2848927-4.patch3.31 KBWim Leers
#2 2848927-2.patch1.8 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2848927-2.patch1.8 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
Issue tags: +Novice, +php-novice

We should update EntityResourceTestBase in a similar way.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
Issue tags: -Novice, -php-novice
FileSize
2848927-4.patch3.31 KB
interdiff.txt1.56 KB

Nobody picking up novice tasks, so then just getting this done.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -435,11 +435,11 @@ public function testGet() {
-    $expected_link_relation_headers = array_map(function ($rel) use ($link_relation_type_manager) {
-      $definition = $link_relation_type_manager->getDefinition($rel, FALSE);
-      return (!empty($definition['uri']))
-        ? $definition['uri']
-        : $rel;
...
+      $link_relation_type = $link_relation_type_manager->createInstance($relation_name);
+      return $link_relation_type->isRegistered()
+        ? $link_relation_type->getRegisteredName()
+        : $link_relation_type->getExtensionUri();

Test coverage shows a new better way of getting $expected_link_relation_headers but tests still pass so looks good to me! RTBC!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Looks like it needs a reroll. Thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
2848927-7.patch3.33 KB

Rerolled. Conflicted with #2852992: EntityResourceTestBase: only assert presence of Link headers for entity types that have link templates. Trivial conflict :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs performance review

From the issue summary:

performance cost that needs to be looked at.

As far as I can see there's no comment that addresses this - is it a concern?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs performance review

REST API performance is not superb, but that's pretty much entirely due to D8's routing and bootstrap (instantiating many services) being so expensive. Not to mention that in this case (a GET request for an entity resource), we have to run entity access and field access.

Besides, the changes only result in very thin value objects being created: see \Drupal\Core\Http\LinkRelationType.

In other words: any performance cost here, which is likely negligible, is absolutely dwarfed by the already poor performance in all other aspects of serving a GET entity resource request.

@effulgentsia wrote which maybe has a performance cost that needs to be looked at. — emphasis mine. He thought that might be the reason for not doing that just yet in the original issue (#2113345: Define a mechanism for custom link relationships). It wasn't. The reason it didn't happen there was because it was simply overlooked in earlier patch iterations.

Finally, benchmarks (not profiling) to back up my argument above:

Command
ab -c 1 -n 100 -H 'Authorization: Basic …' http://d8/node/1?_format=hal_json
Before
Requests per second:    12.38 [#/sec] (mean)
Time per request:       80.758 [ms] (mean)
Time per request:       80.758 [ms] (mean, across all concurrent requests)
Transfer rate:          59.13 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    75   81   3.9     80      97
Waiting:       75   80   3.9     80      97
Total:         75   81   3.9     80      97
After
Requests per second:    12.78 [#/sec] (mean)
Time per request:       78.220 [ms] (mean)
Time per request:       78.220 [ms] (mean, across all concurrent requests)
Transfer rate:          61.05 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    74   78   2.7     77      90
Waiting:       74   78   2.7     77      90
Total:         75   78   2.7     77      90
Conclusion
No noteworthy change.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2848927-7.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Random fail in FormErrorHandlerCKEditorTest, which is completely unrelated.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -391,7 +391,10 @@ public function calculateDependencies() {
+      if ($this->linkRelationTypeManager->getDefinition($relation_name, FALSE)) {

This should be ->hasDefinition() no?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Oh and @Wim Leers thanks for the performance numbers.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Glad they were useful! :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1015 bytes
2848927-15.patch3.32 KB

#12: nice little catch. I wonder how we missed that so far. Anyway, trivial fix :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 7cad23a and pushed to 8.4.x. Thanks!

Credited myself as a reviewer because my feedback affected the code in the patch.

  • alexpott committed 7cad23a on 8.4.x 
    Issue #2848927 by Wim Leers, alexpott: EntityResource::addLinkHeaders()...

Status: Fixed » Closed (fixed)

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