[DRUP-610] Add url as source for apidocs spec file and (DRUP-459) allow re-fetching it afterwards. #2

Jaesin · 2019-05-17T22:51:04Z

Continuation of: apigee/apigee-edge-drupal#191

googlebot · 2019-05-17T22:51:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

Jaesin · 2019-05-17T22:51:23Z

composer.json

src/Entity/ApiDoc.php

mxr576 · 2019-05-20T06:42:04Z

src/Entity/Form/ApiDocReimportSpecForm.php

+        '%label' => $this->entity->label(),
+      ]));
+    }
+    // TODO: $needs_save doesn't tell us if something went wrong.


This is exactly why I suggested earlier to throw an exception when file fetching or saving fails instead of returning a boolean indicator value which is not the OOP solution.

This TODO was added so it can be changed in a follow up task.

Which is the related follow-up task to this? Probably it would be better to create it as a GH issue, because that could be referenced in this PR.
Also, please keep in mind that if this module gets stable before this issue gets fixed then technically, you should not change what it is in SpecFetcherInterface::fetchSpec() without releasing a new major version because that would be a BC breaking change. So any implementation that implements this interface should not throw any exception and should always return a boolean value because this is what it is in the contract.

src/SpecFetcher.php

src/Plugin/Validation/Constraint/ApiDocFileLinkConstraintValidator.php

mxr576 · 2019-05-20T06:55:48Z

src/SpecFetcher.php

+  private function checkRequirements(string $destination): void {
+    // If using private filesystem, check that it's been configured.
+    if (strpos($destination, 'private://') === 0 && !$this->isPrivateFileSystemConfigured()) {
+      throw new \Exception('Private filesystem has not been configured.');


https://github.com/apigee/apigee-edge-drupal/pull/191/files#r284212260

I have no problem with addressing this in a followup PR.

Which is the related follow-up task to this? Probably it would be better to create it as a GH issue, because that could be referenced in this PR.

mxr576 · 2019-05-20T06:57:47Z

src/SpecFetcher.php

+    $this->httpClient = $http_client;
+    $this->entityTypeManager = $entityTypeManager;
+    $this->messenger = $messenger;
+    $this->logger = $logger;


https://github.com/apigee/apigee-edge-drupal/pull/191/files#r283729115

Although this is a micro-optimization, I have no problem addressing it in a followup task.

Which is the related follow-up task to this? Probably it would be better to create it as a GH issue, because that could be referenced in this PR.

mxr576 · 2019-05-20T07:02:58Z

#2 (comment)
The specfetcher service and the automated imported process still do not have a test coverage although these are the most important changes of this PR.

Jaesin · 2019-05-25T00:55:10Z

There are no new tests in this PR. Tests will be added in a followup PR.

Existing test results:

Testing started at 17:47 ...
/usr/local/Cellar/php@7.2/7.2.15/bin/php .../vendor/phpunit/phpunit/phpunit --configuration .../phpunit.xml .../modules/contrib/apigee_api_catalog/tests --teamcity
PHPUnit 6.5.13 by Sebastian Bergmann and contributors.

Testing .../modules/contrib/apigee_api_catalog/tests


Time: 57.47 seconds, Memory: 6.00MB

OK (9 tests, 116 assertions)

Co-Authored-By: Chris Novak <chrisnovak@google.com>

…andler.

…e to spec_file_source and make it list_string type.

…back.

…nstraint.

…ion.

…apigee-edge-drupal/pull/191`.

… `apidoc` entities.

The default value for the `http_errors` option is `TRUE`.

[DRUP-624] Add revisions to API Docs.

….com/Jaesin/apigee-api-catalog-drupal into Jaesin-8.x-1.x-DRUP-610-459-apidocs-add-url # Conflicts: # apigee_api_catalog.links.task.yml # src/Entity/Access/ApiDocAccessControlHandler.php # src/Entity/ApiDoc.php # src/Entity/ApiDocInterface.php # src/Entity/Form/ApiDocForm.php Also updated ApidocEntityTest.php w/ options & file_link modules to get tests to pass.

cnovak

I think this looks good now w/latest updates, should have issues in for anything outstanding. After merging other commits this PR had conflicts. I resolved merge conflicts, then ran phpcs and phpunit and all tests now pass.

However, @Jaesin you should look over ApiDocAccessControlHandler:checkAccess() to make sure that method was properly merged.

Test results:

 ../vendor/bin/phpunit --configuration core modules/contrib/apigee_api_catalog/ 
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing modules/contrib/apigee_api_catalog/
..............                                                    14 / 14 (100%)

Time: 3.36 minutes, Memory: 10.00MB

OK (14 tests, 158 assertions)

src/Entity/ApiDoc.php

Jaesin · 2019-05-27T21:01:45Z

ApiDocAccessControlHandler:checkAccess() looks right to me and all of the access tests pass so it should be good to go.

cnovak added the cla: yes Indicates CLA has been signed label May 17, 2019

mxr576 suggested changes May 20, 2019

View reviewed changes

cnovak added this to the 1.0.0-beta1 milestone May 22, 2019

arlina-espinoza added 12 commits May 22, 2019 14:27

DRUP-624 Add revisions to API Docs.

cdadac2

DRUP-624 Add revisioning tests to API Docs.

09322c7

DRUP-624 Add revisioning permissions with tests to API Docs.

c689b4c

DRUP-624 Add ApiDoc entity settings for default revision.

da963d0

DRUP-624 Fix code sniffer validations.

84f586e

DRUP-624 Add revisions link to local tasks.

fed9ecb

DRUP-624 ApiDocAccessControlHandler implements EntityHandlerInterface.

6c4886c

DRUP-624 Improving code from PR feedback.

96a9f9f

DRUP-624 Improving code from PR feedback.

028ec86

DRUP-624 Improving tests for revisioning.

a2f1bc8

DRUP-624 Fix settings form message for apidocs.

d938e71

DRUP-624 Improve ApiDocSettingsForm.

9a33b38

cnovak added cla: yes Indicates CLA has been signed and removed cla: yes Indicates CLA has been signed labels May 24, 2019

[DRUP-624] Remove the delete permission for apidoc revisions.

993225a

This was referenced May 25, 2019

Tests for PR #2 (url as source for apidocs spec file) #12

Closed

Add tests of API Spec from file source. #13

Open

Follow-ups for PR #2 #14

Closed

Jaesin and others added 5 commits May 24, 2019 21:51

Update apigee_api_catalog.permissions.yml

c3944fd

Co-Authored-By: Chris Novak <chrisnovak@google.com>

Update apigee_api_catalog.permissions.yml

e45bfa1

Co-Authored-By: Chris Novak <chrisnovak@google.com>

[DRUP-624] Translate base field labels.

ca65b24

[DRUP-624] There shouldn’t be a need to specify the default storage h…

f7005aa

…andler.

DRUP-610 Add url as source for apidocs spec file.

9eb78cc

arlina-espinoza and others added 24 commits May 24, 2019 22:52

DRUP-610 Add url as source for apidocs spec file - rename spec_as_fil…

63c671e

…e to spec_file_source and make it list_string type.

DRUP-610 Add url as source for apidocs spec file - addressing PR feed…

2b78af7

…back.

DRUP-459 Refactor API Doc open API spec fetcher as service.

4ae0281

DRUP-610 Add url as source for apidocs spec file - use DI in field co…

55fd0bd

…nstraint.

DRUP-459 Refactor API Doc open API spec fetcher as service - bugfix.

09ebbd4

DRUP-459 API Doc open API spec fetcher - filesystem validations.

61400ea

DRUP-610 Show message when refetching but using file a source.

af8ed99

DRUP-610 Replace file_get_contents() with http_client service.

52cfd07

DRUP-459 API Doc open API spec fetcher - rename stuff.

c4758b7

DRUP-610 Code sniffer fixes.

d5bdecb

[DRUP-610] Fixes the apidoc entity test.

b170d1b

[DRUP-610] Adds access control to the reimport apidoc entity operat…

245de2a

…ion.

[DRUP-610] Rename the spec fetch er service.

078a98b

[Cleanup] Move entity classes under Entity.

7a0f19c

[DRUP-610] Optimizations/suggestions from https://github.com/apigee/…

d141742

…apigee-edge-drupal/pull/191`.

[DRUP-610] Simplify logging sand remove unneeded parameters.

ffd3900

[DRUP-610] No need to log “nothing to do” during fetchSpec for file…

d373b6f

… `apidoc` entities.

[DRUP-610] Adds todo about validation.

d823512

[DRUP-610] Require the file_link module in composer.json.

ec769ac

[DRUP-610] Remove unnecessary trait from the APIDOc entity.

f0db112

[DRUP-610] Use string translation in spec fetcher log messages.

8a0d7d2

[DRUP-610] Remove the deprecated exceptions options.

45caf36

The default value for the `http_errors` option is `TRUE`.

Merge pull request #3 from Jaesin/8.x-1.x-DRUP-624

3b82466

[DRUP-624] Add revisions to API Docs.

cnovak approved these changes May 26, 2019

View reviewed changes

src/Entity/ApiDoc.php Outdated Show resolved Hide resolved

cnovak merged commit 57a5066 into apigee:8.x-1.x May 27, 2019

Jaesin deleted the 8.x-1.x-DRUP-610-459-apidocs-add-url branch May 28, 2019 22:01

cnovak mentioned this pull request Feb 16, 2021

Getting Error on API Documentation settings page #122

Closed

pradeep-developer mentioned this pull request Mar 28, 2024

Unable to configure MariaDB database on Drupal Kickstart Page in Developer portal. #248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRUP-610] Add url as source for apidocs spec file and (DRUP-459) allow re-fetching it afterwards. #2

[DRUP-610] Add url as source for apidocs spec file and (DRUP-459) allow re-fetching it afterwards. #2

Jaesin commented May 17, 2019

googlebot commented May 17, 2019

Jaesin commented May 17, 2019

mxr576 May 20, 2019

Jaesin May 23, 2019

mxr576 May 23, 2019 •

edited

Loading

Jaesin May 25, 2019

mxr576 May 20, 2019

Jaesin May 23, 2019

mxr576 May 23, 2019

mxr576 May 20, 2019

Jaesin May 23, 2019

mxr576 May 23, 2019 •

edited

Loading

mxr576 commented May 20, 2019

Jaesin commented May 25, 2019

cnovak left a comment

Jaesin commented May 27, 2019 •

edited

Loading

[DRUP-610] Add url as source for apidocs spec file and (DRUP-459) allow re-fetching it afterwards. #2

[DRUP-610] Add url as source for apidocs spec file and (DRUP-459) allow re-fetching it afterwards. #2

Conversation

Jaesin commented May 17, 2019

googlebot commented May 17, 2019

Jaesin commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxr576 May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxr576 May 23, 2019 • edited Loading

Choose a reason for hiding this comment

mxr576 commented May 20, 2019

Jaesin commented May 25, 2019

cnovak left a comment

Choose a reason for hiding this comment

Jaesin commented May 27, 2019 • edited Loading

mxr576 May 23, 2019 •

edited

Loading

mxr576 May 23, 2019 •

edited

Loading

Jaesin commented May 27, 2019 •

edited

Loading