-
Notifications
You must be signed in to change notification settings - Fork 0
Add in cacheable json response #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…pe to properly error out with a json response
… to create new types of Reponses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for cacheable JSON responses across the API layer, updating controllers, interfaces, and subscribers, and including new tests and example routes.
- Introduced
CacheableJsonResponseTraitand addedcreateResponse()to the controller interface and base class - Updated
ApiControllerBaseto delegate tocreateResponse()and wired in cache metadata - Added unit and functional tests for cacheable responses, and example implementations and routes
Reviewed Changes
Copilot reviewed 11 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/CacheableJsonResponseTrait.php | New trait to build cacheable JSON responses |
| src/Controller/ApiControllerInterface.php | Added createResponse(): Response to the interface |
| src/Controller/ApiControllerBase.php | Switched to createResponse() and implemented default |
| tests/src/Unit/CacheableJsonResponseTraitTest.php | Unit tests for cache metadata merging and response class |
| tests/src/Functional/ResponseTests.php | Functional test for cacheable pirates endpoint |
| modules/bc_api_example/...CacheableResponsePirateExample.php | Example controller using the cacheable trait |
| modules/bc_api_example/bc_api_example.routing.yml | Routes for cacheable endpoints |
Comments suppressed due to low confidence (4)
src/Controller/ApiControllerInterface.php:87
- Missing import of the
Responseclass. Adduse Symfony\Component\HttpFoundation\Response;at the top of this interface so the return type is recognized.
public function createResponse(): Response;
src/Controller/ApiControllerBase.php:467
- [nitpick] This commented-out instantiation is dead code. Consider removing it to improve readability and reduce clutter.
// $response = new JsonResponse($this->return_data);
tests/src/Functional/ResponseTests.php:91
- [nitpick] The commented-out
drupalLogincall may confuse future maintainers. Remove it or add a comment explaining why login is skipped.
// $this->drupalLogin($user);
tests/src/Functional/ResponseTests.php:115
- [nitpick] Similar to above, this commented login should be documented or removed to keep the test clear.
// $this->drupalLogin($user);
aasarava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow review. Looks great overall. I left some minor/nitpicky notes, but nothing's a blocker.
modules/bc_api_example/src/Controller/ApiControllerCacheableResponsePirateExample.php
Outdated
Show resolved
Hide resolved
modules/bc_api_example/src/Controller/ApiControllerCacheableResponsePirateExample.php
Show resolved
Hide resolved
modules/bc_api_example/src/Controller/ApiControllerCacheableResponsePirateExample.php
Show resolved
Hide resolved
| */ | ||
| public function getResourceQueryResult() { | ||
| // Just having this here as an example. | ||
| // Most times no need to override this method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this comment, because lines 92-94 show a clear reason for overriding the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... ok the non-cacheable one makes sense. But this one doesn't. Esp. since I'm using a trait and not extending the class. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... that "example" should be added to the trait. then the comment will be valid then.
| public function getResourceListQueryResult() { | ||
| // This method should be overridden for any endpoint. | ||
| $query = $this->entityTypeManager->getStorage('node')->getQuery(); | ||
| $query->accessCheck(TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know this is just an example, but should we recommend that accessCheck() be FALSE? otherwise, the results could vary based on which user triggers the API call, right?
Addresses: #15
This pull request introduces significant updates to the
bc_api_examplemodule, focusing on adding cacheable API endpoints, improving response handling, and enhancing test coverage. Key changes include the addition of a new API controller with caching capabilities, the introduction of a reusableCacheableJsonResponseTrait, and updates to tests to validate the new functionality.API Enhancements
/api/cacheable/piratesand/api/cacheable/pirates/{nid}) inbc_api_example.routing.ymlwith authentication and permission requirements. These endpoints are handled by the newApiControllerCacheableResponsePirateExampleclass. [1] [2]CacheableJsonResponseTraitto simplify creating cacheable JSON responses and managing cache metadata. This trait is now used in the new controller.ApiControllerBase) to support cacheable responses by overriding thecreateResponsemethod. [1] [2] [3]Code Improvements
getResourceListQueryResultmethod in the new controller to filter and paginate pirate nodes based on status and type.ApiSubscriberby usinggetPathInfo()for path validation and fixing a logging issue for 404 errors. [1] [2]Testing Enhancements
CacheableJsonResponseTraitTest, to validate the behavior of theCacheableJsonResponseTrait.ResponseTeststo include a new test for the cacheable pirates API endpoint and made minor adjustments to existing tests. [1] [2]Miscellaneous
taxonomymodule inbc_api_example.info.ymlto support filtering by taxonomy ID.classytostable9.