A case can be made for \Drupal\jsonapi\Query\QueryOptionInterface and \Drupal\jsonapi\Normalizer\Value\ValueExtractorInterface for example, since they have many implementations. But CurrentContextInterface, FieldResolverInterface etc clearly have only a single implementation. Perhaps it will make sense to make interfaces for them in the future to allow them to be overridden by contrib modules, but for now, that's far too early.

We should probably do this for #2829327: Review JSON API module for core-readiness — it's highly likely that committers will call out the fact that there are (still) so many interfaces without solid corresponding documentation nor solid justification.

Comments

Wim Leers created an issue. See original summary.

skyredwang’s picture

Status: Active » Needs review
StatusFileSize
new17.22 KB

This patch removed CurrentContextInterfaceand FieldResolverInterface interfaces

wim leers’s picture

Step 1: remove CurrentContextInterface.

wim leers’s picture

StatusFileSize
new20.74 KB
new4.38 KB

Step 2: remove FieldResolverInterface.

wim leers’s picture

StatusFileSize
new24.07 KB
new42.15 KB

Step 3: remove LinkManagerInterface.

wim leers’s picture

StatusFileSize
new5.04 KB
new46.55 KB

Step 4: remove ErrorHandlerInterface + ErrorBase.

wim leers’s picture

StatusFileSize
new5.12 KB
new49.19 KB

Step 5: remove QueryBuilderInterface.

wim leers’s picture

StatusFileSize
new12.6 KB
new60.56 KB

Step 6: remove EntityCollectionInterface.

wim leers’s picture

Step 7: remove DocumentRootNormalizerValueInterface.

wim leers’s picture

StatusFileSize
new6.56 KB
new70.09 KB

Step 8: remove EntityNormalizerValueInterface.

wim leers’s picture

StatusFileSize
new3.06 KB
new72.75 KB

Step 9: remove RelationshipItemNormalizerValueInterface.

wim leers’s picture

StatusFileSize
new4.41 KB
new75.34 KB

Step 10: remove FieldItemNormalizerValueInterface.

wim leers’s picture

StatusFileSize
new4.94 KB
new77.54 KB

Step 11: remove RelationshipNormalizerValueInterface.

wim leers’s picture

Assigned: wim leers » Unassigned

@skyredwang: I'm sorry that I didn't build on top of your patches, but as you can see, the issue was assigned to me. I'd been working on it all this time so I could post a clear set of steps, and green patches only, as you can see in comments #3 through #13.

However, that makes you excellently positioned to review this patch :)

hampercm’s picture

Status: Needs review » Needs work

A few docblock fixes needed:

+++ b/src/Context/FieldResolver.php
@@ -50,7 +59,16 @@ class FieldResolver implements FieldResolverInterface {
+   *   The publicI field name to map to a Drupal field name.

Typo: publicI

+++ b/src/LinkManager/LinkManager.php
@@ -70,7 +92,18 @@ class LinkManager implements LinkManagerInterface {
+   * Get the full URL for a given request object.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request object.
+   * @param array $link_context
+   *   An associative array with extra data to build the links.
+   *
+   * @throws \Drupal\jsonapi\Error\SerializableHttpException
+   *   When the offset and size are invalid.
+   *
+   * @return string
+   *   The full URL.

This docblock seems to be for getRequestLink(). (was incorrect in the interface as well)

+++ b/src/Resource/EntityCollection.php
@@ -56,21 +56,33 @@ class EntityCollection implements EntityCollectionInterface {
+   *   TRUE if the collection has a next page.

no @return

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -135,7 +135,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
-   * @return \Drupal\jsonapi\Normalizer\Value\EntityNormalizerValueInterface
+   * @return \Drupal\jsonapi\Normalizer\Value\DocumentRootNormalizerValue

This looks like the wrong class

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new2.08 KB
new77.73 KB

All of that except the last bit are pre-existing bugs that were just moved. Fixed them all.

The last one is a bug that I already fixed. Look at the logic of that function and what it actually returns :)

e0ipso’s picture

Assigned: Unassigned » e0ipso

Reviewing this

  • e0ipso committed 78bd51e on 8.x-1.x authored by Wim Leers
    fix(Maintainability) Remove all remaining interfaces in the JSON API...
e0ipso’s picture

Assigned: e0ipso » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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