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

Not require SYSTEM #416

Merged
merged 1 commit into from
Jun 30, 2017
Merged

Not require SYSTEM #416

merged 1 commit into from
Jun 30, 2017

Conversation

whikloj
Copy link
Contributor

@whikloj whikloj commented Jun 27, 2017

Github issue: #412

What does this Pull Request do?

Removes the requirement of a SYSTEM section of the config.ini

What's new?

mik required you to have the SYSTEM section just in case your timezone was not set. But conceivably never checked it, it also parsed the settings file twice.

This uses the existing array of settings and only checks for SYSTEM if we need it.

How should this be tested?

I'm going to suggest that if tests run before and after this PR with the same count you should be fine. However my PHPUnit is not letting me run existing tests under PHP 7

But you could try running mik without a SYSTEM section in your config.ini and you should (depending on your PHP installation) get the PHP timezone or 'America/Vancouver' 😄

To help reviewers test your work:

  • Indicate whether your work requires a smoke test or is covered by PHPUnit tests (see CONTRIBUTING.md for more information).

  • If your work is covered by PHPUnit tests, indicate how many successful tests and assertions the reviewer should see when they run your tests.

  • If your work requires a smoke test, provide sample configuration files and data for reviewers.

  • Be as detailed as possible.

  • Good testing instructions and sample confiruation files/data help get your PR completed faster.

    Should be covered by existing tests, but again I haven't been able to run them. There is no new functionality added in here, just a different way of achieving the same thing.

Additional Notes

Any additional information that you think would be helpful when reviewing this PR.

Example:

  • Does this change require documentation to be updated? no
  • Does this change add any new dependencies? no
  • Could this change impact execution of existing code? no

Interested parties

@whikloj
Copy link
Contributor Author

whikloj commented Jun 28, 2017

Schema resolving error in PHP 5.6 on Travis, if someone can restart that test I think it will pass.

@MarcusBarnes
Copy link
Owner

Thanks @whikloj. I've restarted the Travis build.

@MarcusBarnes MarcusBarnes merged commit 0bd2a77 into MarcusBarnes:master Jun 30, 2017
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.

2 participants