Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRUP-574 Simulate pagination if CPS is not enabled. #43

Merged
merged 7 commits into from
Feb 4, 2019
Merged

Conversation

mxr576
Copy link
Contributor

@mxr576 mxr576 commented Jan 30, 2019

Related: apigee/apigee-edge-drupal#123

This implementation keeps BC and also solves the reported issue. Because PHP API client applies a workaround, a shortcut with non-CPS enabled orgs, we should let developers know what happened. This is the reason why I choose to triegger an E_USER_NOTICE.
Introducing APIGEE_EDGE_PHP_CLIENT_SUPPRESS_CPS_SIMULATION_NOTICE environment variables seemed to be a good idea because at this moment E_NOTICE errors can not be suppressed if you are using Drupal, it does not matter what do you configure in the PHP ini.
https://www.drupal.org/project/drupal/issues/1267246

Latest test with real Apigee Edge connection: https://travis-ci.org/mxr576/apigee-client-php/builds/486860650

@mxr576 mxr576 changed the title DRUP-574 Simulate pagination if CPS is not enabled. [WIP] DRUP-574 Simulate pagination if CPS is not enabled. Jan 30, 2019
CPS is not enabled on on-prem installations at this moment.

apigee/apigee-edge-drupal#123
@codecov-io
Copy link

codecov-io commented Jan 31, 2019

Codecov Report

Merging #43 into 2.x will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x      #43      +/-   ##
============================================
+ Coverage     91.49%   91.52%   +0.02%     
- Complexity     1545     1553       +8     
============================================
  Files           240      240              
  Lines          2999     3033      +34     
============================================
+ Hits           2744     2776      +32     
- Misses          255      257       +2
Impacted Files Coverage Δ Complexity Δ
src/Exception/CpsNotEnabledException.php 0% <ø> (-50%) 2 <0> (ø)
src/Controller/PaginationHelperTrait.php 100% <100%> (ø) 33 <26> (+8) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e23f5e...3e099d3. Read the comment docs.

@mxr576 mxr576 changed the title [WIP] DRUP-574 Simulate pagination if CPS is not enabled. DRUP-574 Simulate pagination if CPS is not enabled. Jan 31, 2019
@mxr576
Copy link
Contributor Author

mxr576 commented Feb 1, 2019

Tested it with private cloud build v4.19.01, worked as expected. Both client and Drupal 8 tests passed.
@cnovak Would you like to review this PR and approve it before I merge?

Copy link
Collaborator

@cnovak cnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General thoughts:

  • Paging simulation: I am not sure if paging simulation will work on non CPS orgs. I thought I remember that items are not guaranteed to come back in same order. I sent an email to persons that can answer this and cc'd you @mxr576

  • I am not sure how the E_USER_NOTICE error surfaces in our Drupal modules. Just want to make sure this message is suppressed without extra user work since non CPS orgs are standard for private cloud users and probably will always be that way.

  • If a user is using this library against a non CPS org and not trying to use paging there shouldn't be any difference. If this is true, we could throw exception or error only when trying to use a pager on a nonCPS org (that could also be suppressed). This way we only expose that something may be behaving differently than expected in that very narrow case.

Since we should solve this ASAP, we can put this PR in now and look at this again if needed.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mxr576
Copy link
Contributor Author

mxr576 commented Feb 4, 2019

@cnovak Thanks for the review, I could not commit your suggestion in one commit by using GH's new suggestions feature so I have to commit them in the usual way.

Reacting to your comments:

  1. The answer that we got is pretty much unclear and if what it suggest is true we would need to run sorting on the result of a CPS supported listing call and not on the non-CPS ones. Nothing is mentioned about what should be the sorting parameter when objects and not ids are returned. Because we haven't seen any issues yet that would possible caused by the missing sorting I'd leave the code as it is .

  2. Because the paginated result on a non-CPS org is generated by PHP and not by the MGMT API server I think we should communicate that to developers. As I wrote in the description I could not find a better way to do that, I had experienced with multiple possible solutions. Setting up environment variables is usually quite easy and straightforward in any environments, so this seemed to be the best solution. Also, it is up to the server admin/devops guy on a project whether E_NOTICE level warnings gets logged in a production environment, if they suppressed this "info" never gets logged in production.
    If we get complains about this in the Drupal 8 module we can find a way to provide a configuration UI/automatically suppress this notice.

  3. E_USER_NOTICE error is triggered only when someone tries to use the pagination feature on a non-CPS org, if no pager is defined no error is triggered on a non-CPS org.

@mxr576 mxr576 merged commit 8a0a1ca into apigee:2.x Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants