basz / SlmLocale

Auto detection of locale through different strategies for Zend Framework 2
Other
68 stars 34 forks source link

fix #26 - feature/phpunit #99

Closed koseduhemak closed 6 years ago

koseduhemak commented 6 years ago

Fixing issue #26.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.09%) to 82.461% when pulling e83340642737057a31aa61ba46b8e0e4ed8dbe8f on koseduhemak:feature/phpunit into 2a8a0a66eced2cdddd60462ef62aa3df62002653 on basz:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.09%) to 82.461% when pulling 63a8195d73b1af0728639a1ee30523eaddbcc63c on koseduhemak:feature/phpunit into 2a8a0a66eced2cdddd60462ef62aa3df62002653 on basz:develop.

koseduhemak commented 6 years ago

fixed travis by bumping version of "zendframework/zend-modulemanager" depedency to "^2.8.2". Please have a look if we can now merge.

koseduhemak commented 6 years ago

any news regarding this pull request?

basz commented 6 years ago

missed this one. seems reasonable i guess...

koseduhemak commented 6 years ago

Thank you for merging, when will it available in master, to let me / people pull it from composer?

basz commented 6 years ago

not sure, @svycka?

however for now you could point to the developer branch in composer until it is released i think

"require": {
        "slm/locale": "dev-developer"
    }
svycka commented 6 years ago

I don't like this workaround but also don't have much time to fix this issue so we can create release

koseduhemak commented 6 years ago

I agree with you, @svycka . Problem is though that there is no easy fix, because of the short-circuit of the event processing while creating the application (getApplication()). However, with this fix the phpunit tests working again, so one can use this module in combination with custom phpunit tests (extending AbstractControllerTestCase) only that way at the moment. Otherwise all such tests will fail and break testability.

svycka commented 6 years ago

maybe it was possible to add strategy for tests with higher priority to prevent other strategies like uripath? Okay just guessing but I hope there is a better way to do this.

The biggest problem here is that when we remove this workaround this will be BC break not a good thing for workaround :D

But again I don't have time at the moment so if this is a problem then we can do it like this if we don't have other options.

koseduhemak commented 6 years ago

@svycka: Ok I will try to add a custom strategy which does the fix... I would propose that I create another pr if I finished and we leave this one as it is (if the strategy approach is not working). We can revert before merge of this pr, if strategy approach works :D

What do you think, is this a good way to go?

svycka commented 6 years ago

I guess it would be better as it would not require any code in this library this could only be added to documentation only

koseduhemak commented 6 years ago

Please have a look at #100