fix(mongodb): Exception throwing fatal error, Broken handling of Mong…

[josefsabl]

…odbConnectionFactory config, Incorrect MongodbConnectionFactory documentation

Closes: #1031, #1027

[makasim]

There are some CS issues (check Travis for more details). Could you take a look at it? Good to go once fixed.

[josefsabl]

Yes, I have seen it. But am pretty confused as these violations are on lines I did NOT touch.
ALSO By looking at the code in the editor the code looks well formatted while format "suggested" by PHP CS Fixer looks funky to me.
ALSO I noticed there is the only job with PHP CS Fixer applied, isn't it forgotten or mal-configured or something? :-)

[Steveb-p]

Yes, I have seen it. But am pretty confused as these violations are on lines I did NOT touch.

CI is configured to look if files need fixing in terms of code style if they have been modified. They might have been in this state since some time, and since no one ever updated those, no longer conform to code style guidelines.

[josefsabl]

Maybe, but the formatting it suggest looks broken. Do you really want me to break the formatting of code I didn't even author?

[Steveb-p]

The changes that PHP-CS-Fixer suggest are "only" those:

--- Original
+++ New
@@ @@
         if (false == isset($parsedUrl['scheme'])) {
-            throw new \LogicException(sprintf(
-                'The given DSN schema "%s" is not supported. There are supported schemes: "%s".',
-                $parsedUrl['scheme'],
-                implode('", "', array_keys($supported))
-            ));
+            throw new \LogicException(sprintf('The given DSN schema "%s" is not supported. There are supported schemes: "%s".', $parsedUrl['scheme'], implode('", "', array_keys($supported))));
         }

--- Original
+++ New
@@ @@
             if (!is_int($delay)) {
-                throw new \LogicException(sprintf(
-                    'Delay must be integer but got: "%s"',
-                    is_object($delay) ? get_class($delay) : gettype($delay)
-                ));
+                throw new \LogicException(sprintf('Delay must be integer but got: "%s"', is_object($delay) ? get_class($delay) : gettype($delay)));
@@ @@
             if (!is_int($timeToLive)) {
-                throw new \LogicException(sprintf(
-                    'TimeToLive must be integer but got: "%s"',
-                    is_object($timeToLive) ? get_class($timeToLive) : gettype($timeToLive)
-                ));
+                throw new \LogicException(sprintf('TimeToLive must be integer but got: "%s"', is_object($timeToLive) ? get_class($timeToLive) : gettype($timeToLive)));
             }

I understand that it's not your fault or concern - I can fix that for you if you prefer, and we will merge your PR on top of mine - it will just take a few hours more to get it merged.

I'd like you to understand why it happened and why you were asked for it. Historically there was no PHP-CS used in enqueue-dev, but at some point it was introduced. Due to the fact that the code base was relatively large it was decided to not fix code style immediately and instead postpone it - PHP-CS-Fixer is specifically set up to only check files that changed in the branch in question.

Obviously as you can tell it was wrong and code style should have been fixed then and there, but the choice was made and we have to live with it. If it's any consolation for you I wasn't present here in this repo at the time as well, so I had no influence over this.
I also would prefer those exceptions to still be on separate lines, since I think they are more readable as they are (and don't violate the soft line length limit too), but unless you want to try and tweak PHP-CS settings now we have to just conform to the current settings.

So, while it's not specifically your code, we will have to adjust anyway. Ignoring CI checks is worse offense in my view than adjusting a couple lines that are unrelated to your issue 😞

[josefsabl]

All understood.

If you are okay with me changing parts of code that are not related to the issue and you are also okay with inferior formatting (And you already replied positively to both these concerns) I have no problem with that either :-) Will do.

By this nagging, I only wanted to make absolutely clear that the problem is understood and this debate won't accidentally do more harm that good. I also consider this concert dismissed.

[josefsabl]

On a completely unrelated note. Is it okay, that composer install generated some files for me in the bin directory that want to get commited? I will not do it of course, but isn't the .gitignore set up incorrectly?

[Steveb-p]

isn't the .gitignore set up incorrectly?

Highly possible 😄

[josefsabl]

isn't the .gitignore set up incorrectly?

Highly possible 😄

Okay, I won't dare to try to fix this here as I may break something.