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

fix #26 - feature/phpunit #99

Merged
merged 5 commits into from
Dec 12, 2017
Merged

fix #26 - feature/phpunit #99

merged 5 commits into from
Dec 12, 2017

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