[WebProfilerBundle] fixed parsing Mongo DSN and added Test for it #10146

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

malarzm commented Jan 27, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10105
License MIT
Doc PR
Contributor

malarzm commented Jan 27, 2014

There's 1 Failure only for PHP 5.3 (https://travis-ci.org/symfony/symfony/jobs/17702052#L501) - is it Monday and the test started in one second and finished in another and that's the error?

Member

stof commented Jan 27, 2014

don't worry about this particular test. We merged a fix for such timing failures in the 2.4 branch, but the 2.4 branch has not yet been merged into master since this fix.

Contributor

malarzm commented Jan 27, 2014

Ok, thanks :)

@fabpot fabpot and 1 other commented on an outdated diff Jan 28, 2014

...ponent/HttpKernel/Profiler/MongoDbProfilerStorage.php
@@ -117,6 +114,29 @@ protected function getMongo()
}
/**
+ * Internal convenience method for parsing DSN
+ *
+ * @param string $dsn
+ *
+ * @return null|array Array($server, $database, $collection)
+ */
+ protected function parseDsn($dsn)
@fabpot

fabpot Jan 28, 2014

Owner

this should be private

@malarzm

malarzm Jan 28, 2014

Contributor

Can't do that since I'm calling it from DummyMongoDbProfilerStorage in Test

@fabpot

fabpot Jan 28, 2014

Owner

We do not mark methods protected just for the tests. Use the reflection to make it accessible.

@malarzm

malarzm Jan 28, 2014

Contributor

Sorry, I was suggested by getMongo that is protected and later used in mentioned class as public, will do

@fabpot fabpot commented on an outdated diff Jan 28, 2014

...ponent/HttpKernel/Profiler/MongoDbProfilerStorage.php
@@ -117,6 +114,29 @@ protected function getMongo()
}
/**
+ * Internal convenience method for parsing DSN
+ *
+ * @param string $dsn
+ *
+ * @return null|array Array($server, $database, $collection)
+ */
+ protected function parseDsn($dsn)
+ {
+ if (preg_match('#^(mongodb://.*)/(.*)/(.*)$#', $dsn, $matches)) {
@fabpot

fabpot Jan 28, 2014

Owner

Can change this to the negative:

if (!preg_match('#^(mongodb://.*)/(.*)/(.*)$#', $dsn, $matches)) {
    return null;
}

@fabpot fabpot commented on an outdated diff Jan 28, 2014

...ponent/HttpKernel/Profiler/MongoDbProfilerStorage.php
@@ -117,6 +114,29 @@ protected function getMongo()
}
/**
+ * Internal convenience method for parsing DSN
+ *
+ * @param string $dsn
+ *
+ * @return null|array Array($server, $database, $collection)
+ */
+ protected function parseDsn($dsn)
+ {
+ if (preg_match('#^(mongodb://.*)/(.*)/(.*)$#', $dsn, $matches)) {
+ $server = $matches[1];
+ $database = $matches[2];
+ $collection = $matches[3];
+ preg_match('#^mongodb://(([^:]+):?(.*)(?=@))?@?([^/]*)(.*)$#', $server, $matchesServer);
+ if (empty($matchesServer[5]) && !empty($matches[2]))
@fabpot

fabpot Jan 28, 2014

Owner

You must add the {} even for one liners.

@fabpot

fabpot Jan 28, 2014

Owner

Also, we are not using empty() but "real" comparisons.

@fabpot fabpot commented on an outdated diff Jan 28, 2014

...pKernel/Tests/Profiler/MongoDbProfilerStorageTest.php
@@ -87,6 +92,36 @@ public function testCleanup()
self::$storage->purge();
}
+ public function testDsnParser()
+ {
+ $tests=array(
@fabpot

fabpot Jan 28, 2014

Owner

I would use a data provider instead.

@fabpot fabpot commented on an outdated diff Jan 28, 2014

...ponent/HttpKernel/Profiler/MongoDbProfilerStorage.php
@@ -117,6 +114,29 @@ protected function getMongo()
}
/**
+ * Internal convenience method for parsing DSN
@fabpot

fabpot Jan 28, 2014

Owner

Does it bring any value, so should be removed.

@malarzm @malarzm malarzm [WebProfilerBundle] fixed parsing Mongo DSN and added Test for it
Applied Coding Standards patch

Applied Fabien's comments

[WebProfilerBundle] fixed parsing Mongo DSN and added Test for it

Removing whitespaces

Applied fabbot patch
5cd274b
Owner

fabpot commented Feb 21, 2014

Thanks @malarzm.

@fabpot fabpot added a commit that referenced this pull request Feb 21, 2014

@fabpot fabpot bug #10146 [WebProfilerBundle] fixed parsing Mongo DSN and added Test…
… for it (malarzm)

This PR was submitted for the 2.3-dev branch but it was merged into the 2.3 branch instead (closes #10146).

Discussion
----------

[WebProfilerBundle] fixed parsing Mongo DSN and added Test for it

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10105
| License       | MIT
| Doc PR        |

Commits
-------

5cd274b [WebProfilerBundle] fixed parsing Mongo DSN and added Test for it
a8955bf

fabpot closed this Feb 21, 2014

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