Add entity type only schema routes for entity types with bundles [#2870904]

Thanks so much for this module.
I am working on the OpenAPI module. Currently it has some code I copied over from the previous version of this module but I would like to remove it and rely on this module.
#2870393: Remove openapi_json_schema submodule and add dependency on Schemata module
I can almost use this module expect that you can longer produce schema for just and entity type if the entity also supports bundles.

This is needed when creating the OpenAPI spec because the REST resources don't really point to specific bundle(unlike JSON API).

This patch will add a route for all entity types regardless of whether they also supports bundles.

I am attaching Open API JSON file produced with patch to this module.
If look at the definitions section of this json you will see a definition for node, node:article and node:page. This is possible with changes to this module.

Comment	File	Size	Author
#22	2870904-22.patch	54.94 KB	tedbow
#22	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass
#21	2870904-21-including-2871167-28.patch	115.87 KB	tedbow
#21	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass
#21	2870904-21-do-not-test.patch	55.15 KB	tedbow
#21
#19	2870904-19-including-2871167-25.patch	88.52 KB	tedbow
#19	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass
#19	2870904-19-do-not-test.patch	42.55 KB	tedbow
#19
#16	2870904-16.patch	25.9 KB	tedbow
#16	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 2 fail PHP 5.5 & MySQL 5.5, D8.3 2 fail PHP 5.5 & MySQL 5.5, D8.2 2 fail
#15	2870904-15.patch	25.9 KB	tedbow
#15	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 No tests found
#12	2870904.patch	51.32 KB	Grayside
#12	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Patch Failed to Apply
#9	15.patch	150.48 KB	Grayside
#9	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Patch Failed to Apply
#8	15.patch.txt	150.48 KB	Grayside
	definitions.png	22.24 KB	tedbow
	openapi.json_.txt	11.27 KB	tedbow

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

19 April 2017 at 01:35

tedbow created an issue. See original summary.

Comment #2

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 19 April 2017 at 01:53

Status:

Active

» Needs review

Pull request: https://github.com/phase2/schemata/pull/15

Comment #3

Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commented 19 April 2017 at 21:35

Status:

Needs review

» Needs work

I have reviewed the PR, and there were a couple minor bits that need work.

Comment #4

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 28 April 2017 at 19:10

Status:

Needs work

» Needs review

@Grayside thanks for looking at the PR. I update the PR and pushed new commits that address your concerns I think.

Comment #5

jludwig CreditAttribution: jludwig at SciShield commented 28 April 2017 at 19:27

There is discussion in the PR about whether to add a new route for each bundle or to change it to one or two routes with paths like this:

/schemata/{entity_type}/{bundle}

What is in the current PR works fine, and whether to change that or not requires a discussion because it may affect things like local tasks. This is important issue so my suggestion would be to get this PR in first and open another issue to discuss whether to use a dynamic path and less routes.

Comment #6

Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commented 3 May 2017 at 04:58

Agreed, handling the routes more dynamically is a separate issue.

Comment #7

Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commented 3 May 2017 at 05:03

Status:

Needs review

» Needs work

Comment #8

Grayside CreditAttribution: Grayside at Phase2 commented 30 May 2017 at 18:04

File	Size
15.patch.txt	150.48 KB

Copied the patch version of the Github PR to see if I can get the tests to run.

Comment #9

Grayside CreditAttribution: Grayside at Phase2 commented 30 May 2017 at 18:05

File	Size
15.patch	150.48 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Patch Failed to Apply

Comment #10

Grayside CreditAttribution: Grayside at Phase2 commented 30 May 2017 at 18:07

Status:

Needs work

» Needs review

Comment #11

30 May 2017 at 18:09

Status:

Needs review

» Needs work

The last submitted patch, 9: 15.patch, failed testing.

Comment #12

Grayside CreditAttribution: Grayside at Phase2 commented 30 May 2017 at 18:12

File	Size
2870904.patch	51.32 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 Patch Failed to Apply

Comment #13

Grayside CreditAttribution: Grayside at Phase2 commented 30 May 2017 at 18:12

Status:

Needs work

» Needs review

Comment #14

30 May 2017 at 18:15

Status:

Needs review

» Needs work

The last submitted patch, 12: 2870904.patch, failed testing.

Comment #15

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 30 May 2017 at 19:25

Status:

Needs work

» Needs review

File	Size
2870904-15.patch	25.9 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 No tests found

Ok here is patch from that same PR.

Comment #16

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 30 May 2017 at 19:45

File	Size
2870904-16.patch	25.9 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 2 fail PHP 5.5 & MySQL 5.5, D8.3 2 fail PHP 5.5 & MySQL 5.5, D8.2 2 fail

Namespace was wrong in RequestTest so test was not found.

Comment #17

30 May 2017 at 19:49

Status:

Needs review

» Needs work

The last submitted patch, 16: 2870904-16.patch, failed testing.

Comment #18

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 30 May 2017 at 20:21

Status:

Needs work

» Needs review

Retesting against core 8.3.x.
8.4.x will not work until #2871167: Replace rest.link_manager with hal.link_manager and add a dependency on hal is fixed.

UPDATE: actually that change was in 8.3.x too so testing against 8.2.x

Comment #19

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 8 June 2017 at 12:18

Version:

8.x-1.0-alpha1

» 8.x-1.x-dev

File	Size
2870904-19-do-not-test.patch	42.55 KB

2870904-19-including-2871167-25.patch	88.52 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass

Ok not that #2871167: Replace rest.link_manager with hal.link_manager and add a dependency on hal is hopefully close to be done.

Here a patch that adds test changes from that issue and also adds the expectations for the new routes created by this issue.

Uploading 2 patches. 1 with both issues changes to get the tests to pass and 1 with just this issues changes.

Comment #20

dawehner

German

CreditAttribution: dawehner as a volunteer commented 9 June 2017 at 15:32

+++ b/src/Routing/Routes.php
@@ -71,29 +71,79 @@ class Routes implements ContainerInjectionInterface {
+      // If this entity type supports bundles then add a route for each bundle.
+      if ($entity_type->getBundleEntityType()) {

Well, technically every entity type supports bundles, just some of them don't have an entity type connected to it, see \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo. Is this restriction strictly needed?

Comment #21

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 9 June 2017 at 18:31

File	Size
2870904-21-do-not-test.patch	55.15 KB

2870904-21-including-2871167-28.patch	115.87 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass

@dawehner thanks for the review

+++ b/src/Routing/Routes.php
@@ -71,29 +71,79 @@ class Routes implements ContainerInjectionInterface {
-      $has_bundle = (bool) $entity_type->getBundleEntityType();

The previous code was still using $entity_type->getBundleEntityType() to determine which entity types should get a route for every bundle so the this functionality remains the same.

I will update that comment though to be more accurate.

Comment #22

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 12 June 2017 at 23:21

File	Size
2870904-22.patch	54.94 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.4 1 pass

rerolled after #2871167: Replace rest.link_manager with hal.link_manager and add a dependency on hal

Comment #23

tedbow

he/him

English

Ithaca, NY, USA

CreditAttribution: tedbow at Acquia commented 13 June 2017 at 12:15

Created new pull request https://github.com/phase2/schemata/pull/22 , sorry for inconvience

Comment #24

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 15 June 2017 at 20:49

Status:

Needs review

» Reviewed & tested by the community

I reviewed this in GitHub already, forgot to update here too.

Comment #25

Grayside CreditAttribution: Grayside at Phase2 commented 26 June 2017 at 16:07

Status:

Reviewed & tested by the community

» Fixed

Awesome to see test coverage continue to expand on this.

I manually tested that the schemas generate with and without a bundle, and all looks green there as well as the automated tests here.
I merged the Github PR.

Per the comment near the bottom of the Github PR, there are a couple issues in the backlog for automated schema validation:

Comment #26

Grayside CreditAttribution: Grayside at Phase2 commented 26 June 2017 at 16:10

Another potentially relevant follow-up is #2798019: Deduplicate property names and centralize entity properties, wherein we consider the value of modifying schema output for entities with bundles to leverage a separate schema for common elements.