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

Change environment variables to use getenv #30

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

tractorcow
Copy link

Replaces #28

@ichaber
Copy link

ichaber commented Nov 19, 2017

Thanks for implementing the necessary changes @tractorcow 🌟

@chillu
Copy link
Member

chillu commented Nov 20, 2017

@tractorcow There's some PHPCS failures, could you have a look? Charlie is approving the changes (he's sitting next to me), so feel free to self merge - unless you think it needs someone else's review. This is critical for SSP 4.x operation, high priority

@tractorcow
Copy link
Author

tractorcow commented Nov 20, 2017

Ok fixed. Also moved to vendormodule.

composer.json Outdated
@@ -15,6 +15,7 @@
}
],
"require": {
"silverstripe/vendor-plugin": "^1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this module expose front-end resources? If not, I don't think this module is a requirement.

Also, are we expecting that modules are allowed to rely on framework supplying this as a requirement, or do we expect that every module maintainer has to add this line? I'd suggest that relying on framework to properly take care of the implications of using silverstripe-vendormodule would be an acceptable division of responsibility.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, removed.

Modules without exposed resources don't need the plugin.

@chillu chillu merged commit e9b714c into silverstripe:master Nov 20, 2017
@chillu chillu deleted the feature/4 branch November 20, 2017 04:21
@chillu
Copy link
Member

chillu commented Nov 20, 2017

I've also published a 4.0.1 release: https://github.com/silverstripe/silverstripe-dynamodb/releases/tag/4.0.1, and adjusted the branch alias to match the main release line: #31

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