CommentFileSizeAuthor
#11 2842639--fix-docblocks--11.patch37.26 KBe0ipso
#9 2842639--interdiff--3-9.txt6.47 KBe0ipso
#9 2842639--fix-docblocks--9.patch37.04 KBe0ipso
#3 jsonapi_clean_up_all_class_docblocks-2842639-3.patch44.08 KBwim leers
#3 interdiff.txt7.04 KBwim leers
#2 jsonapi_clean_up_all_class_docblocks-2842639-2.patch38.28 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Clean up all class docs: ensure all have @internal, none have @package » [PP-1] Clean up all class docs: ensure all have @internal, none have @package
Assigned: wim leers » Unassigned
Status: Active » Postponed
StatusFileSize
new jsonapi_clean_up_all_class_docblocks-2842639-2.patch38.28 KB

And patch.

This is relative to #2842378: Remove all remaining interfaces in the JSON API module that have only one implementation, so postponing on that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt7.04 KB
new jsonapi_clean_up_all_class_docblocks-2842639-3.patch44.08 KB

Oops, I forgot to add the final bits of work.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Title: [PP-1] Clean up all class docs: ensure all have @internal, none have @package » Clean up all class docs: ensure all have @internal, none have @package
Status: Postponed » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay, green :)

hampercm’s picture

hampercm commented
Assigned: Unassigned » hampercm

Reviewing...

hampercm’s picture

hampercm commented
Assigned: hampercm » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Needs work

I think that the normalizers should not be internal. This is the main tool implementors can use to add their custom properties and alter the input/output of the API (by creating normalizers that extend from ours).

Thoughts?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs work » Needs review
StatusFileSize
new 2842639--fix-docblocks--9.patch37.04 KB
new 2842639--interdiff--3-9.txt6.47 KB

This is what I was proposing in #8.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think that's okay for now, as long as JSON API would be alpha-experimental in Drupal core, because everything is allowed to change anyway.

But marking everything @internal is purely a preventative measure: it means we explicitly signal everything can still change. Not marking something @internal means you're free to extend from it for your own needs, and then if we change the logic of, say, the Field or ContentEntity normalizer, that could then break this custom normalizer. Which means we must FOREVER keep the normalizers work exactly the same. That is what I'm protecting against.

Of course, contrib/custom modules are free to add their own normalizers. I just want to make sure they don't depend on JSON API's normalizers.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed
StatusFileSize
new 2842639--fix-docblocks--11.patch37.26 KB

Re-rolled and merged.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

  • e0ipso committed d4c2840 on 8.x-1.x authored by Wim Leers 
    fix(Maintainability) Clean up all class docs: ensure all have @internal...

Status: Fixed » Closed (fixed)

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