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

Apc Driver: Check for 'apc.enable_cli' in cli calls #368

Merged
merged 3 commits into from Aug 24, 2017

Conversation

Biont
Copy link
Contributor

@Biont Biont commented Aug 21, 2017

As described in #365, Stash can currently run into a fatal error in specific circumstances.

This PR introduces a check in Apc->isAvailable() to let the driver fail early if APCu is not configured to run in CLI environments.

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage decreased (-0.07%) to 95.468% when pulling cd196fd on Biont:master into e9a6aaf on tedious:master.

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage decreased (-0.07%) to 95.468% when pulling cd196fd on Biont:master into e9a6aaf on tedious:master.

@alexbowers
Copy link
Contributor

Can you confirm that this works as expected, both with and without APC enabled?

@Biont
Copy link
Contributor Author

Biont commented Aug 21, 2017

I did test it locally both with and without apc.enable_cli in my php config, yes.

@Biont
Copy link
Contributor Author

Biont commented Aug 21, 2017

Hm, is there anything I need to do to make Travis happy? I checked for tests of the isAvailable method and found none.

Since ini_get() is not testable and introducing a config container for abstraction seemed out of scope, I did not touch unit tests.

@tedivm
Copy link
Member

tedivm commented Aug 23, 2017

@Biont - the test that's failing is the style check. You just need to fix the indentation on your code and it should pass.

@alexbowers - feel free to merge this once it passes.

@alexbowers
Copy link
Contributor

@tedivm Is it the style?

Could be worth splitting up between StyleCI and Travis CI for style and unit tests?

@tedivm
Copy link
Member

tedivm commented Aug 23, 2017

Yeah, when I set the project up StyleCI didn't exist so I just added a test case in for styling. If you want to break that up that would be great.

@alexbowers
Copy link
Contributor

alexbowers commented Aug 23, 2017 via email

@tedivm
Copy link
Member

tedivm commented Aug 23, 2017

Yeah- it's all in the contributing docs.

@tedivm
Copy link
Member

tedivm commented Aug 23, 2017

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 95.468% when pulling 0ae4b37 on Biont:master into e9a6aaf on tedious:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 95.468% when pulling 0ae4b37 on Biont:master into e9a6aaf on tedious:master.

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage decreased (-0.07%) to 95.468% when pulling 0ae4b37 on Biont:master into e9a6aaf on tedious:master.

@Biont
Copy link
Contributor Author

Biont commented Aug 24, 2017

Thanks for the hint @tedivm I enabled PSR2 codesniffing for this project and should be good to go now, should there be future PRs

@alexbowers alexbowers merged commit df07131 into tedious:master Aug 24, 2017
@alexbowers
Copy link
Contributor

Thanks @Biont.

@tedivm merged.

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.

None yet

4 participants