Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unescape parameters in the installation bundle #1037

Merged
merged 5 commits into from Dec 4, 2019
Merged

Conversation

@leofeyer
Copy link
Member

leofeyer commented Dec 2, 2019

Fixes #979

@leofeyer leofeyer added the defect label Dec 2, 2019
@leofeyer leofeyer added this to the 4.8 milestone Dec 2, 2019
@leofeyer leofeyer requested review from ausi, Toflar, bytehead and aschempp Dec 2, 2019
@leofeyer leofeyer self-assigned this Dec 2, 2019
Copy link
Member

Toflar left a comment

Seems pretty important to me so I'd say it deserves a unit test 😊

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 3, 2019

@Toflar Turns out we cannot unit test this, because the Plugin class does not use DI and I cannot mock the DriverManager::getConnection() call.

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Dec 3, 2019

And what do you want to tell me now? 😄 We should find a way to make it testable then. Maybe you could pass a mock class using the driverClass option? Wdyt?

Copy link
Contributor

aschempp left a comment

As discussed, we should use $container->resolveEnvPlaceholder(…, true)

@leofeyer leofeyer dismissed stale reviews from ausi and bytehead via 6a670d7 Dec 3, 2019
@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 3, 2019

Changed in 6a670d7. Now the code also works with a DATABASE_URL environment variable.

@leofeyer leofeyer requested review from aschempp, bytehead and ausi Dec 3, 2019
@leofeyer leofeyer requested a review from Toflar Dec 3, 2019
Copy link
Member

Toflar left a comment

Smartass :) It's a bit hacky but it's a very welcome unit test and serves the purpose. Nice one!

Copy link
Contributor

aschempp left a comment

Unit-testing internal methods is always wrong. DriverManager::getConnection is already a factory, so that's very easy to mock!

public function __constructor($connectionFactory = [DriverManager::class, 'getConnection']);

$connection = call_user_func($connectionFactory, $params);
Copy link
Contributor

aschempp left a comment

I have updated the PR accordingly 😎

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 3, 2019

I am against these changes. The unit test is supposed to test the parameter resolving, not the database connection. There is absolutely no need to inject a dependency here, especially since it is only used in the unit tests.

@leofeyer leofeyer dismissed stale reviews from Toflar and aschempp via d2cb958 Dec 3, 2019
@Toflar
Toflar approved these changes Dec 3, 2019
Copy link
Member

Toflar left a comment

'- ignore -

@aschempp aschempp requested review from Toflar and aschempp Dec 3, 2019
Copy link
Member

Toflar left a comment

Injecting the connection factory is correct. And if code adjustments are needed for unit tests it means that the class did not follow software architecture best practices before. That's clearly the case here.

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 3, 2019

Ok, you know what? We have 54 open issues for Contao 4.9 and only three weeks left to close them. I have more important things to do than to nitpick about an unimportant unit test. I am going to release Contao 4.8 without the fix and we can discuss this in January then. 🤷‍♂

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Dec 3, 2019

Your unit test is covering 9% of the file. It tests that the parameters are correctly resolved. But it does not test that that's related to passing the extension configuration, or that the database is correctly initialized.

leofeyer

My tests cover 19% of the file, including the DB connection. What's still missing is the extension handling, we should add another test to check if the return value is correct and that it returns version 5.5 on exception. Adding that test will not be possible with your unit test approach.
aschempp

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 4, 2019

Of course your tests covers more than mine, because you are now adding tests for previously uncovered stuff (such as the DB connection). 🤦‍♂

My unit test only tests exactly what has been changed in this PR.

I agree that we should add tests for the uncovered stuff. There are probably other uncovered methods, too. But please do this in a separate PR instead of highjacking this one.

@leofeyer leofeyer force-pushed the bugfix/unescape-parameter branch from d2cb958 to 69e6cfd Dec 4, 2019
@leofeyer leofeyer force-pushed the bugfix/unescape-parameter branch from 69e6cfd to c262c81 Dec 4, 2019
@leofeyer leofeyer requested a review from Toflar Dec 4, 2019
@Toflar
Toflar approved these changes Dec 4, 2019
Copy link
Member

Toflar left a comment

Neat! Thank you!

@leofeyer leofeyer merged commit 595aa5e into 4.8 Dec 4, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@leofeyer leofeyer deleted the bugfix/unescape-parameter branch Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.