search

chesio / bc-security

Helps keeping WordPress websites secure.
The Unlicense
14 stars 3 forks source link

Clean up global instance variable #126

Closed szepeviktor closed 2 years ago

szepeviktor commented 2 years ago

Breaks this line https://github.com/chesio/bc-security/blob/37483924ff2b9ae4221d1b6ee236c552e396172c/tests/integration/src/Bootstrap.php#L69-L70

@chesio What do you think?

chesio commented 2 years ago

@szepeviktor That's a hard one to be honest.

I don't like polluting of global namespace, but I don't like code duplication even more, so that's why I want to reuse the bootstrap code in bc-security.php also for integration tests - with the exception that any plugin code that actually integrates with WordPress (= code that calls add_action and add_filter) should not be executed during integration tests bootstrap.

That's why I refactored the plugin bootstrapping in daab48fc1c8459456a5ee290abda0236a5c8006c - to be able to unhook the load method during integration test bootstrap and call it later in setUp method of PHPUnit. It's a bit of trickery, but WordPress doesn't make unit testing testing easy.

One alternative that I already considered is to have this conditional handling directly in bc-security.php file, but somehow I don't like it. The load method should be invoked in webserver context and WP-CLI context and perhaps some other contexts as well, just not in context of integration tests. So I find the unhook approach very clean, despite that it produces global variable as a side effect. Btw. I think the variable is global only in normal (webserver) context, not in WP-CLI and not in integration tests.

szepeviktor commented 2 years ago

I see. It is hard to make decisions.

szepeviktor commented 2 years ago

BTW It seems like the lack of dependency injection. I solve it with a static immutable configuration object. https://github.com/szepeviktor/small-project/blob/master/src/Config.php

chesio commented 2 years ago

I see. It is hard to make decisions.

Anyway, you got me thinking (again) - see d8ecafc1df5f4d9695c129138d31cf780e373c66. Thanks!