Skip to content

Conversation

@koseduhemak
Copy link
Contributor

Fixing issue #26.

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@koseduhemak
Copy link
Contributor Author

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

@koseduhemak
Copy link
Contributor Author

any news regarding this pull request?

@basz
Copy link
Owner

basz commented Dec 12, 2017

missed this one. seems reasonable i guess...

@basz basz merged commit 5c272ac into basz:develop Dec 12, 2017
@koseduhemak
Copy link
Contributor Author

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

@basz
Copy link
Owner

basz commented Dec 12, 2017

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
Copy link
Collaborator

svycka commented Dec 12, 2017

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

@koseduhemak
Copy link
Contributor Author

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
Copy link
Collaborator

svycka commented Dec 12, 2017

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
Copy link
Contributor Author

@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
Copy link
Collaborator

svycka commented Dec 12, 2017

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
Copy link
Contributor Author

Please have a look at #100

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants