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

Already on GitHub? Sign in to your account

[ZF-11899] S3: Buckets With Capital Letters Not Supported #1201

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants

Partial solution of the issue. For full support of US General bucket names, SOAP API calls are required instead of REST ones. Is it worth?

Also added the $s3->getLastResponse() method for debugging and for complex applications.

Duccio Gasparri added some commits May 10, 2012

[ZF-11899] Buckets With Capital Letters Not Supported
Amazon\S3\S3: Added A-Z to the regular expression of _validBucketName()
Amazon\AbstractAmazon: added $this->_lastResponse and getLastResponse()
for debugging and advanced application construction
Amazon\S3\S3: added support for getLastResponse() method


Change-Id: I38d0d45b2ae0c9b5e15f3839a997a2fd39b1fafa
[ZF-11899] S3: Buckets With Capital Letters Not Supported
Partially upgraded the class to allow for US General bucket names.

Updated _validBucketName() and addSignature(), now accepting when
appropriate IP addresses names and dashes

However, the full support of US General bucket names is possible only by
switching to SOAP Api calls. Is it worth doing?

For reference:
http://docs.amazonwebservices.com/AmazonS3/latest/dev/BucketRestrictions.html

http://framework.zend.com/issues/browse/ZF-11899

Change-Id: I6d3b06618faf73d1256b7a23f9e6eafbe27123d9

Also removed a bug in S3::_makeRequest($method, $path='', $params=null, $headers=array()) on line 674 (duplicated code) of commit 49b1d99

Owner

weierophinney commented May 11, 2012

Could you provide some unit tests for this, by any chance? I want to ensure we don't break existing behavior, but also that we don't revert the updates in the future.

I'm studying right now what @Maks3w has suggested me as a model for unit tests, and what's already available (ZendTest\Service\Amazon\S3\OnlineTest.php). I never wrote unit tests for services. But right now I'm still fighting with the class to get everything up and working (I don't know yet Amazon response to each call to create an offline test).

I wouldn't be worried about breaking existing behaviors, as the class just didn't work. I'm honestly wondering if it is worth to switch to SOAP, as the only unsupported feature of REST is the US General upper case & empty label bucket names, something that also Amazon discourages.

Duccio Gasparri added some commits May 11, 2012

[ZF2-281] && [ZF2-262] Cannot Count rows in a MySQLi Result Set /
Zend\Db\Adapter\Driver\Mysqli\Result Iterator implementation mangled

Cleaned both Result.php and ResultSet.php and made them compliant with
the Iterator interface

1) fixed the count() issue
2) fixed the 2+ next() issue (2 or more next() did not advance mysqli
pointer)
3) fixed the issue with rewind() and mysqli losing fetched data, by
buffering mysqli results
4) simplified Result.php by eliminating the currentComplete and
nextComplete, both aggregated into currentData = null | false | array
5) removed caching of count() in ResultSet.php (row count is
already stored in Result.php)
6) fixed a issue with the logic of pointer (pointer can be changed only
by next() and rewind(), and should not be touched by current())



Change-Id: I5bf38cd6adb5330deff0400428db3d6ded651efc

My commit d8027ac was added to this pull request by mistake, but it is anyway final. It solves issues http://framework.zend.com/issues/browse/ZF2-262 and http://framework.zend.com/issues/browse/ZF2-281 (both about the Myiqli Result Iterator.

Duccio Gasparri added some commits May 12, 2012

Free resources after data has been buffered
Change-Id: Ic30af85af610cb5e841fe5f7b665dadcbc137327
Partial online test for ResultTest
Lacking a mockup of \mysqli_stmt, working on it

Change-Id: Id0f91f5f33323e33c241d76d69dae8ac2677e4ae
Zend\Db\Adapter\Driver\Mysqli\ResultTest
Fixed directory structure, use and small changes


Change-Id: I9eb2c3d2e6c0cef56ee4ff5bc2e0c4ead648b86b

Ok, here's my issue with the Mockup object. On line 215 or Db\Adapter\Driver\Mysqli\Result.php there's this line:

call_user_func_array(array($this->resource, 'bind_result'), $this->statementBindValues['values']);

Is it possible to emulate this behavior with a mock of mysqli_stmt? It takes $this->statementBindValues['values'] by reference and populate it with the values of the row

Member

Maks3w commented May 12, 2012

@ralphschindler Maybe you should take a look to this PR.

@dgasparri You should check where that value is set and then mockup the first call to the dependency. With a first look seems to be the line 206.

@Maks3w , found what I was looking for. bind_result method gets its arguments as reference, and populates them:

bool mysqli_stmt::bind_result ( mixed &$var1 [, mixed &$... ] )

The return of the function is only true/false(error). I wasn't sure how to build a Mockup for that behavior. I found what I was looking for sebastianbergmann/phpunit-mock-objects#81 , and will try to build the Test this weekend

But unfortunately this feature is still signed as an open bug in PHPUnit, so it will work only after the framework has been fixed.

Member

Maks3w commented May 12, 2012

@dgasparri Maybe we can avoid the need to use mockups for this. I will do some tests with this http://about.travis-ci.org/docs/user/database-setup/

@Maks3w that would solve the problem.If there's the possibility to create a temporary table (or anyway a table) so to use \mysqli, the test is then quite straightforward

Member

Maks3w commented May 12, 2012

Other different way that you can try is create your own mysqli class extending from the original \mysqli and override the methods to test.

That's also a good idea, definitely better than creating a mockup of mysqli (I drafted it and it's too complicated). I'm documenting myself over PHPUnit and the various options (it's a quite extended framework). It all comes down to what is "allowed" and "not allowed" in the ZF2 tests, and that's something I can't answer. There's also the DBUnit http://www.phpunit.de/manual/3.7/en/database.html , which is somehow part of unit test framework.

If a database access could be granted (which I'm starting to suppose considering the Travis link you sent me and the TravisTestConfiguration in the tree folder), then the more realistic solution is to use it (maybe via DBUnit). Otherwise we just extend the mysqli class and subclasses.

Is there already a direction set by the team? (and/or by previous tests?) Is there a document outlining those issues?

Member

ralphschindler commented May 13, 2012

Theres way too much going on inside this pull request. At first glance, it appears to be about Amazon S3, but then drifts into Zend\Db? Can you please split these up into separate pull requests?

As a side note, buffering inside the result object is a bad idea. The philosophy of ZF2's Zend\Db is to not do anything unexpected and magical between the developer and the db server. In general, Zend\Db will favor prepared statements over direct execution of sql... and it will favor driver level defaults, which means unbuffered before buffered in terms of Mysqli (this is perhaps one of the reasons people use mysqli over PDO_Mysql). Basically, we shouldn't be buffering unless the developer explicitly opt-ed into this behavior.

Here's what I think should happen: In a situation where you are using all the defaults, calling count() on a Mysqli Result that is unbuffered should throw an exception. In that case, the developer could then calls $result->store() to buffer the result. count() will then work after store() had been called (no exception). There could also be a Connection level option to always buffer results that is also passed in as an option to newly created Results.

@ralphschindler "Theres way too much going on inside this pull request". My fault, I'm still trying to figure out how to separate pull requests in github and git. On a side note, it's for sure the future, but reading from the joomla developers groups, people spend more time fighting with git rather than coding.

If we want to mantain compatibility with the Iterator interface expected behavior, there should be some buffering at one level or another. Without buffering, I cannot count results and cannot do two or more consecutive foreach on the same result set - something people expect from the Iterator interface, and that's the reason of the two jira requests and of my working on it.

We have the following options:

  1. buffering at the very beginning with mysqli_stmt::result_store() method (cannot be done later because fetched rows are lost). Pros: fully compatible with the Iterator interface. Cons: resource-hungry, which can be a problem especially on large queries;

  2. buffering in the Result class at the very beginning: all the ups and downs of point 1, but for sure less efficient and more error prone than the php built-in method;

  3. buffering in the Result class at runtime (as it is my commit). Pros, fully compatible with the Iterator; cons: as you said, it adds a layer, and at the end of the set, memory usage is the same as if we would have store results at the beginning (there's an advantage only for result sets that are not fully retrieved)

  4. buffering the Statement instead of the results, so that if somebody would like to go through the set a second time, we can just run again the query and recreate a new mysqli_stmt to feed in. Pros: doesn't waste resources, and it doesn't add any unexpected behavior. Cons: the count() method still doesn't work; and this is really an issue of the driver chosen, for example mysql (non-i) wouldn't have it, so ideally it should be transparent to the rest of Zend\Db and of the Framework, and should be handled inside of the Driver\Mysqli namespace.

  5. not declare compatibility with the Iterator interface for Result and ResultSet, so that people don't expect the count() and rewind() methods (this is not really an option, as the ResultSet could not be used anymore with the foreach)

or 6) as you suggested - the most reasonable - to have a form of opt-in and opt-out, along with one of the points 1-4

I like your philosophy of Zend\Db. So:

  1. Zend\Db should do the very minimum between the db server and the developer
  2. Zend\Db should be easy to understand and use, and it should be compliant with the php standard library

I imagine that if I want or don't want buffering, it's something I choose at the very beginning, so switching during runtime will throw an Exception.
So: ---- BUFFERING: done via mysqli_stmt (the most efficient solution)
--- NO BUFFERING: data are sent directly to the output without being stored, count() will throw an exception, the first rewind() is ignored if done at the beginning (necessary for compability with foreach), and subsequent rewind() or a rewind() not at the beginning of the cycle will throw an exception

Is it a satisfactory solution? (I should be able to code it by the end of the day)

And for the opt-in or opt-out, I take a wild guess: 95% of the developers that will use ZF will be non-masters of php, handling small projects that are not resource-critical or involving out-of-the-ordinary operations. 5% of developers will need flexibility to adapt the Framework behavior to their wills. What do you think if buffering is activated by default, so by default it is compatible with Iterator and foreach, and won't force unexperienced users to understand why their "$numRows = count($rows); for($i = 0; $i<$numRows; $i++)" piece of code doesn't work?

And if I want to disable buffering, it should be

$resultSet = $adapter->select()->buffering(false);

Member

ralphschindler commented May 13, 2012

@dgasparri As for the git workflow, the people in IRC channel #zftalk.2 can help you out. Basically, whatever you're attempting to fix should have a branch of its own, then you create a pull request from that branch in your repository to master.

One thing you keep implying is that Iterators require that count work. This is not the case. This is why there is an Iterator interface and a Countable interface. Throwing an exception when we can't reasonably discern the actual length of the result-set is the most proper and PHP-like thing to do IMO. Then, you opt-into behavior that will make the length known, like storing the result-set to a buffer or (another option which we've not discussed) issuing a query to determine the count. The point being is that there are a few different strategies to employ given a particular situation and given a particular RDBMS backend.

This is one of my top priorities in the next week. If you are online and in IRC, let's chat there (i am ralphschindler on Freenode).

In the mean time, I am going to close this pull request, as I believe you really need to get the S3 stuff into a branch and issue a pull request against that. At this point, I'd rather not see someone merge this thinking they are merging the S3 stuff and have it affect Zend\Db unknowingly.

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