Correct the "Bottom line" minimum requirements check #23

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

I received a bug report which demonstrated that the _cli.php compatibility test can return conflicting results as the conditions used to check for the minimum requirements being satisfied are different.

The conflicting results are shown http://drupal.org/node/1429614#comment-5593470.

The change should make them patch up in the same way they are in the web script.

Contributor

skyzyx commented Feb 16, 2012

There are (roughly) 3 stages that we try to address in this test.

  • Absolutely yes
  • Mostly yes
  • No

Which one of these are you trying to address, and what is wrong with how it currently is? For example, DOM is now required, but you've removed it from the test.

Could you clarify?

I looks as thought I may have incorrectly changed the condition. The issue is that the two conditions that check minimum requirements in the cli file do not match. Please see the following.

From the results I referenced:

Your environment meets the minimum requirements for using the AWS SDK for PHP!

* The OpenSSL extension is installed. This will allow you to use CloudFront
Private URLs and decrypt Windows instance passwords.

* The Zlib extension is installed. The SDK will automatically leverage the
compression capabilities of Zlib.

* You're running on a 32-bit system. This means that PHP does not correctly
handle files larger than 2GB (this is a well-known PHP issue).

* Storage types available for response caching:
The file system, Memcache

----------------------------------------

Bottom Line: We're sorry...
Your PHP environment does not support the minimum requirements for the
AWS SDK for PHP.

You will note the seemingly contradictory statements

  • "Your environment meets the minimum requirements for using the AWS SDK for PHP!"
  • "Your PHP environment does not support the minimum requirements for the AWS SDK for PHP."

As for the correct condition I would assume you are correct in that it should include dom. Another issue I had to workout with the script was that there is no way to pragmatically check for compatibility short of a string search. Perhaps we can clean these scripts up a bit by pulling out the common bits and the evaluating the long conditions for the three cases once and simply checking them below.

Perhaps https://github.com/boombatower/aws-sdk-for-php/compare/compatibility-overhaul would work nicely as it removes the duplicate parts and puts them in one place so inconsistencies such as this shouldn't happen. Also it allows third-parties like myself to load the inc.php file and simply check the compatibility level programmatically. I attempted to follow your file naming convention and what not, but I'm sure we can change things to your liking if you like the approach.

Contributor

skyzyx commented Feb 23, 2012

Yeah, this is better. Let me run it up the flagpole for review. :)

Any update on this?

Contributor

skyzyx commented Mar 29, 2012

We got tied up with the release of PHP for Elastic Beanstalk. Sorry about that.

We're going over the SDK issue tracker this week and merging/fixing everything in our queue. We'll merge this soon. :)

ecaron commented Mar 30, 2012

I also think that the compatibility checking for cURL should be extended to performing an actual connection and verifying that curl_exec !== false. I would do a pull request for this, but want to wait to see the final form this merge takes so I can figure out the right place for it to land.

(The reason the full cURL check is necessary is some Linux flavors bundle cURL with an incomplete NSS library and that prevents cURL for successfully completing SSL connections. Only after verifying the round-trip works can this be ruled out or identified as the culprit. Addressing this in the requirements check will drastically cut back on the curl error code 52/60 that hit the forums.)

Contributor

jeremeamia commented Apr 5, 2012

I made the changes from the compatibility-overhaul branch into my working copy. It should be going out soon in an upcoming release. Thanks Jimmy.

@jeremeamia jeremeamia closed this Apr 5, 2012

Thanks, this was a real pain in the neck when trying to use nagios for status checks and having this flag incorrectly that it wasn't compatible. I appreciate you getting it in.

Re-factored my integration code to take advantage of this change and it is much simpler and straight forward. http://drupal.org/node/1429614#comment-5916966

Thanks for getting this in.

Contributor

jeremeamia commented Apr 25, 2012

Cool! Thanks for helping us improve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment