Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed PHP-805: Deprecate the "chunks" option in MongoGridFS::__construct #435

Closed
wants to merge 1 commit into from

3 participants

@derickr
Owner

I've also made the message in MongoDB::getGridFS() the same.

@derickr derickr Fixed PHP-805: Deprecate the "chunks" option in MongoGridFS::__construct
I've also made the message in MongoDB::getGridFS() the same.
ba8da6c
@jmikola jmikola commented on the diff
@@ -198,7 +198,7 @@ static int php_mongo_command_supports_rp(zval *cmd)
return;
}
if (arg2) {
- php_error_docref(NULL TSRMLS_CC, MONGO_E_DEPRECATED, "This argument doesn't do anything. Please stop sending it");
@jmikola Owner
jmikola added a note

The old message is sassy :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bjori bjori commented on the diff
gridfs/gridfs.c
@@ -86,6 +86,10 @@
return;
}
+ if (chunks) {
+ php_error_docref(NULL TSRMLS_CC, MONGO_E_DEPRECATED, "The 'chunks' argument is deprecated and ignored");
@bjori Owner
bjori added a note

No, its not ignored. It is very much still used in both the if (files) block and the else.

@derickr Owner
derickr added a note

That's how it looks like at first glance, but it's not actually the case. In the if:

        MAKE_STD_ZVAL(chunks);
        spprintf(&temp, 0, "%s.chunks", Z_STRVAL_P(files));
        ZVAL_STRING(chunks, temp, 0);

The original "chunks" is simply overwritten there... and the same here in the else:

    } else {
        …
        MAKE_STD_ZVAL(chunks);
        ZVAL_STRING(chunks, "fs.chunks", 1);

I first thought that that would cause a memory leak, but that is not true. The passed in chunk is automatically freed when the function ends (As it's part of function arguments) and as we don't change the original pointer's value (we just change chunks to point to something new), there is no memleak. The new chunks zval is freed later on.

@bjori Owner
bjori added a note

lol. Thats a total wtf :)

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

LGTM

  • verify the argument isn't used internally either
@derickr derickr referenced this pull request from a commit
@derickr derickr Merged pull request #435 db5da9a
@derickr derickr closed this
@timothy-r timothy-r referenced this pull request in purekid/mongodm
Merged

Removes obsolete second parameter in call to getGridFS #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 8, 2013
  1. @derickr

    Fixed PHP-805: Deprecate the "chunks" option in MongoGridFS::__construct

    derickr authored
    I've also made the message in MongoDB::getGridFS() the same.
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 2 deletions.
  1. +1 −1  db.c
  2. +5 −1 gridfs/gridfs.c
  3. +17 −0 tests/generic/bug00805.phpt
View
2  db.c
@@ -198,7 +198,7 @@ PHP_METHOD(MongoDB, getGridFS)
return;
}
if (arg2) {
- php_error_docref(NULL TSRMLS_CC, MONGO_E_DEPRECATED, "This argument doesn't do anything. Please stop sending it");
@jmikola Owner
jmikola added a note

The old message is sassy :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ php_error_docref(NULL TSRMLS_CC, MONGO_E_DEPRECATED, "The 'chunks' argument is deprecated and ignored");
}
object_init_ex(return_value, mongo_ce_GridFS);
View
6 gridfs/gridfs.c
@@ -76,7 +76,7 @@ static zval* insert_chunk(zval *chunks, zval *zid, int chunk_num, char *buf, int
Creates a new MongoGridFS object */
PHP_METHOD(MongoGridFS, __construct)
{
- zval *zdb, *files = 0, *chunks = 0, *zchunks;
+ zval *zdb, *files = NULL, *chunks = NULL, *zchunks;
zval *z_w = NULL;
/* chunks is deprecated */
@@ -86,6 +86,10 @@ PHP_METHOD(MongoGridFS, __construct)
return;
}
+ if (chunks) {
+ php_error_docref(NULL TSRMLS_CC, MONGO_E_DEPRECATED, "The 'chunks' argument is deprecated and ignored");
@bjori Owner
bjori added a note

No, its not ignored. It is very much still used in both the if (files) block and the else.

@derickr Owner
derickr added a note

That's how it looks like at first glance, but it's not actually the case. In the if:

        MAKE_STD_ZVAL(chunks);
        spprintf(&temp, 0, "%s.chunks", Z_STRVAL_P(files));
        ZVAL_STRING(chunks, temp, 0);

The original "chunks" is simply overwritten there... and the same here in the else:

    } else {
        …
        MAKE_STD_ZVAL(chunks);
        ZVAL_STRING(chunks, "fs.chunks", 1);

I first thought that that would cause a memory leak, but that is not true. The passed in chunk is automatically freed when the function ends (As it's part of function arguments) and as we don't change the original pointer's value (we just change chunks to point to something new), there is no memleak. The new chunks zval is freed later on.

@bjori Owner
bjori added a note

lol. Thats a total wtf :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
if (files) {
zval *temp_file;
char *temp;
View
17 tests/generic/bug00805.phpt
@@ -0,0 +1,17 @@
+--TEST--
+Test for PHP-805: Deprecate the "chunks" option in MongoGridFS::__construct.
+--SKIPIF--
+<?php require "tests/utils/standalone.inc";?>
+--FILE--
+<?php
+require_once "tests/utils/server.inc";
+$mongo = mongo_standalone();
+$db = $mongo->selectDB(dbname());
+
+$gridfs = new MongoGridFS($db, "foo", "bar");
+$gridfs = $db->getGridFS("foo", "bar");
+?>
+--EXPECTF--
+%s: MongoGridFS::__construct(): The 'chunks' argument is deprecated and ignored in %sbug00805.php on line %d
+
+%s: MongoDB::getGridFS(): The 'chunks' argument is deprecated and ignored in %sbug00805.php on line %d
Something went wrong with that request. Please try again.