Branch next major: version 2, requiring Drupal core >=8.5 [#2952293]

The LinkManager has been deprecated for a while now though. In fact, I think at one point you agreed with me that it should be @internal, but it seems we only marked it @deprecated instead.

Log in or register to post comments

Comment #8

gabesullice

English

commented 21 March 2018 at 23:48

While we're on the topic of "things we'd like to break"...

GET /jsonap/node/article/uuid

ought to be

GET /jsonap/node--article/uuid

The latter is much more conventional and will unify our resource type names across the module. This would also make the entrypoint a little nicer.

The former continues to imply that we treat 'articles' and 'pages' as a child of 'node' rather than as siblings.

This breaks the spec recommendation to treat URLs as though they're they're part of a reference document.

I believe the current implementation is a holdover from early days of the module when we had shakier UUID support and no concept of a ResourceType.

^{Edit: Adding a more radical idea (because it's almost 1am and I'm feeling impulsive) s/node--article/node:article/g simply because it's aesthetically pleasing}

Log in or register to post comments

Comment #9

e0ipso

Catalan

Can Picafort

commented 22 March 2018 at 05:40

#8: I'd like to separate that one since it's incredibly disruptive to ALL existing installations without gaining us a feature or a fixing a bug. Maybe open a new issue for discussion?

#7:

The LinkManager has been deprecated for a while now though. In fact, I think at one point you agreed with me that it should be @internal, but it seems we only marked it @deprecated instead.

Yeah. My comment was more on the lines of what to recommend instead, and not let's hold that off. However I do feel we should not recommend using route names to generate links. They are an internal implementation detail that is not guaranteed to stay as is. IMO toUrl should be the way to go.

Log in or register to post comments

Comment #10

gabesullice

English

commented 22 March 2018 at 07:54

I'd like to separate that one since it's incredibly disruptive to ALL existing installations without gaining us a feature or a fixing a bug. Maybe open a new issue for discussion?

Ah! I should have said that this could easily be handled with a BC flag. Then it wouldn't break any sites.

A few features this would buy us: conventions, less confusion, following a spec recommendation, compatibility with existing JS clients like Orbit.js which almost always template the URL by using the type name.

#2949635: Make the resource type aware of the resource path is what have me the hope that we could do this.

I'm a little surprised because JSON API Extras has been altering the path for quite some time (although not with the new method). In fact, it's probably one of it's most used features.

I don't think it's necessary to go into a separate issue, that implies a really long running discussion. Which I really don't think is necessary.

This brings up a good point though, we should try our best to make the upgrade path as smooth and painless as possible.

Yeah. My comment was more on the lines of what to recommend instead, and not let's hold that off. However I do feel we should not recommend using route names to generate links. They are an internal implementation detail that is not guaranteed to stay as is. IMO toUrl should be the way to go.

👍

Log in or register to post comments

Comment #11

wim leers

Ghent 🇧🇪🇪🇺

commented 23 March 2018 at 18:13

#8: I like it, but it's massively disruptive of course.

#10: handling this with a BC flag is doable. But … you say that this gets us more conventions, less confusion. This is only going to be true for those that don't have this BC layer enabled. For everybody else, there's going to be confusing/conflicting documentation!
Now I'm going to contradict myself: it's actually even possible to provide BC for this without a BC flag: with a InboundPathProcessorInterface implementation, much like \Drupal\rest\PathProcessor\PathProcessorEntityResourceBC, we could just rewrite incoming URLs and have this work transparently! We can then remove it in a far future version, for example version 3. Something like this should work:

class JsonApiPathBC implements InboundPathProcessorInterface {

  /**
   * {@inheritdoc}
   */
  public function processInbound($path, Request $request) {
    if (strpos($path, '/jsonapi/') === 0) {
      $parts = explode('/', $path);
      // Rewrite ap old JSON API paths to new ones:
      // - from: /jsonapi/node/article/UUID
      // - to: /jsonapi/node:article/UUID
      if (count($parts) === 4) {
        return sprintf('/%s/%s:%s/%s', $parts[0], $parts[1], $parts[2], $parts[3]);
      }
    }
  }

}

Log in or register to post comments

Comment #12

e0ipso

Catalan

Can Picafort

commented 23 March 2018 at 18:22

I believe fiercely on no BC shenanigans, specially for no gain. I still don't see what benefit this brings. I think it's just fine as it is and should not be modified.

Log in or register to post comments

Comment #13

wim leers

Ghent 🇧🇪🇪🇺

commented 23 March 2018 at 18:26

I think I agree with you, @e0ipso. The benefit seems very small. Until we hear people actually complain about this, this is probably fine.

Log in or register to post comments

Comment #14

wim leers

Ghent 🇧🇪🇪🇺

commented 23 March 2018 at 18:41

Issue summary:

View changes

We will also be able to fix #2933895: [>=8.5] Update type hint to interface instead of concrete class in FieldResolver and #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX, and likely more still.

Log in or register to post comments

Comment #15

wim leers

Ghent 🇧🇪🇪🇺

commented 23 March 2018 at 18:48

So did we agree that LinkManager should be marked @internal in the next major?

Log in or register to post comments

Comment #16

e0ipso

Catalan

Can Picafort

commented 23 March 2018 at 22:28

#15 yes, I believe so.

Log in or register to post comments

Comment #17

gabesullice

English

commented 25 March 2018 at 16:32

Just gonna chime in and say I'm fine with not doing #8. I still think it would be nice. But it's small enough to say, "forget about it" :)

Log in or register to post comments

Comment #18

bojanz commented 26 March 2018 at 17:37

Why would dropping support for unmaintained Drupal core versions require a new major?
I have enforced the "current core branch" requirement on most my modules (Commerce, Address) with no problems.

Log in or register to post comments

Comment #19

e0ipso

Catalan

Can Picafort

commented 26 March 2018 at 20:14

@bojanz we support all the supported versions, which is IIRC >=current-1. In any case the major includes a good bunch of other stuff as well, not just the core requirements.

I'm interested to hear your impressions on upgrade speed from site owners. Are they upgrading core quickly from your perception?

BTW it's awesome to see you around this issue queue.

Log in or register to post comments

Comment #20

gabesullice

English

commented 26 March 2018 at 20:25

FTR only, prior minor versions are not supported.

Previous minor releases will become unsupported when a new minor release is published.

See: https://www.drupal.org/core/release-cycle-overview

I only know this now because I heard @xjm say this was a common misconception the other day.

IDC about whether we support only the current or the previous one too.

Log in or register to post comments

Comment #21

wim leers

Ghent 🇧🇪🇪🇺

commented 30 March 2018 at 09:59

Apparently the 8.x-2.x branch was created?

Log in or register to post comments

Comment #22

wim leers

Ghent 🇧🇪🇪🇺

commented 30 March 2018 at 10:00

Issue summary:

View changes

Per #2843147-51: Add JSON:API to core as a stable module, we need to do #2957271: [>=8.5] Fix RouteEnhancerInterface deprecation errors.

Log in or register to post comments

Comment #23

wim leers

Ghent 🇧🇪🇪🇺

commented 4 April 2018 at 10:51

@dawehner explicitly asked in #2843147-60: Add JSON:API to core as a stable module to do #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it and remove the jsonapi.entity.to_jsonapi service before JSON API is moved into core. This further stresses the need for v2.

Log in or register to post comments

Comment #24

wim leers

Ghent 🇧🇪🇪🇺

commented 4 April 2018 at 15:03

And #2958504: Stop using jsonapi.entity.to_jsonapi service in ResourceTestBase is an issue that paves the path for

Move EntityToJsonApi service to jsonapi_extras to consolidate the feature and have more flexibility to evolve that API. This will also have the benefit to remove the only public service from the module, leaving all the PHP API as internal.

Log in or register to post comments

Comment #25

wim leers

Ghent 🇧🇪🇪🇺

commented 4 April 2018 at 15:51

Issue summary:

View changes

#2957474: Move the write functionality of config entities to a sub-module in preparation for removal was created for

Move config entity POST/PATCH/DELETE support to jsonapi_extras (because not ready for prime time): #2887313: JSON API indicates it supports POST/PATCH/DELETE of config entity types, but that's impossible

Log in or register to post comments

Comment #26

wim leers

Ghent 🇧🇪🇪🇺

commented 5 April 2018 at 10:42

Version:

8.x-1.x-dev

» 8.x-2.x-dev

#2958504: Stop using jsonapi.entity.to_jsonapi service in ResourceTestBase already landed :)

Log in or register to post comments

Comment #27

wim leers

Ghent 🇧🇪🇪🇺

commented 26 April 2018 at 10:33

I think #2942549: Spec Compliance: JSON API allows POSTing relationship fields in 'attributes' rather than in 'relationships' is perhaps another relatively minor bug that we should only fix in 2.x, to minimize breakage (despite our clear statements in jsonapi.api.php about spec compatibility).

Log in or register to post comments

Comment #28

gabesullice

English

commented 26 April 2018 at 11:17

#27++

Log in or register to post comments

Comment #29

wim leers

Ghent 🇧🇪🇪🇺

commented 26 April 2018 at 14:24

Issue summary:

View changes

Alright, updated #2942549: Spec Compliance: JSON API allows POSTing relationship fields in 'attributes' rather than in 'relationships''s metadata, and updated IS.

Log in or register to post comments

Comment #30

wim leers

Ghent 🇧🇪🇪🇺

commented 7 May 2018 at 15:13

Issue summary:

View changes

#2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible also requires >=8.5, so moved it to v2 and added to IS.

Log in or register to post comments

Comment #31

wim leers

Ghent 🇧🇪🇪🇺

commented 28 June 2018 at 01:03

Issue summary:

View changes

#2982210: Move EntityToJsonApi service to JSON API Extras is now the actual issue for

Move EntityToJsonApi service to jsonapi_extras to consolidate the feature and have more flexibility to evolve that API. This will also have the benefit to remove the only public service from the module, leaving all the PHP API as internal.