Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Address @dawehner's feedback in https://www.drupal.org/node/2843147#comment-11872253.
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff-2844373-9-10.txt | 13.72 KB | hampercm |
#10 | address_core_feedback-2844373-10.patch | 27.8 KB | hampercm |
|
Comments
Comment #2
e0ipsoPasting feedback for readability
This is just some review while going through some of the code.
Note: We don't yet use markdown files in core ... I'm fine with adding some, its much easier to read.
I guess we talk about 8.3.x as of this core patch.
Nitpick: Afaik we use
<a href="">:url</a>
instead of generating links, in help texts.For easier readability: Could we reorder the entries in this file to be ordered by priority somehow?
Nice!
Wow, its sad that we don't use the controller resolver, so we cannot easily pull values out of the request attributes.
Can we point to the place in the space where this is defined?
Note: We could use
->get("_json_api_params[$parameter_key]", NULL, TRUE)
instead of this issetIs there a reason why these exceptions are in the Error and not Exception namespace?
As somewhere else on the patch, this could use
$request->attributes->get('_route_params[_json_api_params]', NULL, TRUE)
<3 mini pagers!
... When
$field_name
doens't exist, it throws an exception, according to the documentation on\Drupal\Core\Entity\FieldableEntityInterface::get
I'm wondering whether we should check for
FieldableEntityInterface
instead, as these are the methods called hereDon't we have this service injected?
Ubernitpick: What's the reason for this dash?
Just for sanity reason I would also use $strict=TRUE as the third parameter.
Nitpick: You can also use
array_reduce($this->includes, 'array_merge', [])
here.Isn't it just sad how OOP code just doesn't really compose together. Note: You could use
array_walk($includes, [$this, 'addCacheableDependency'])
to save some lines.Wow, I had no idea that this is actually possible
I wonder whether we should use
return $has_child || $group_id == $id | $group->hasChild($id)
I thought PHP actually lazy evaluates in this case.
Can we document using
EntityInterface[]
I couldn't find any usecase for this, was this basically meant as abstract constant or so?
Wow, that's cool
Comment #3
hampercm CreditAttribution: hampercm at Acquia commentedWorking on a patch...
Comment #4
hampercm CreditAttribution: hampercm at Acquia commentedAddressed first block of code review comments from #1:
Comment #5
hampercm CreditAttribution: hampercm at Acquia commentedUgh, bad patch uploaded...
Comment #6
hampercm CreditAttribution: hampercm at Acquia commentedCorrect patch for #4:
Addressed first block of code review comments from #1:
1) Leaving README.md as-is for now.
2) Change this in the core patch only, as the module is supported on 8.2.x?
3) Fixed
4) Reordered
7) Add @see to PHPDoc
8) Changed as suggested
10) Changed as suggested
12) Replaced with try/catch and added test.
13) Leaving as-is for clarity, since this check is verifying both are content entities.
14) Fixed
5, 6, and 11 didn't require any changes (I think, please verify @e0ipso!)
9) needs further consideration: move the Exception classes into an Exception namespace, and leave the ErrorHandler where it is?
Patch for second block of comments in #1 still to come.
Comment #7
e0ipsoThe patch in #6 looks good!
Comment #8
dawehnerYeah totally ...
You know, I just like things to be named what they are :)
Note: Once we have committed it to core we should certainly try to get us up on http://jsonapi.org/implementations/
Thank you! That's much better
Comment #9
hampercm CreditAttribution: hampercm at Acquia commentedAddressing second block of comments from #1:
1) Needs to be addressed in the core patch.
2) Done
3) This seems to break something. Leaving as-is.
4) Simplified.
6) Simplified.
7) PHPDoc adjusted
8) Yes, this appears to document that constant being needed in all classes that extend JsonApiParamInterface. I added a bit more documentation explaining it.
Nothing to be done for 5 and 9.
Comment #10
hampercm CreditAttribution: hampercm at Acquia commentedMove exception classes to
\Drupal\jsonapi\Exception
namespace.All above comments that can be addressed here have been.
Comment #11
hampercm CreditAttribution: hampercm at Acquia commentedComment #12
dawehnerCool, thank you!
Comment #13
e0ipso