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 DatabaseStore binary safety #50

Closed

Conversation

dnsl48
Copy link
Contributor

@dnsl48 dnsl48 commented Jun 28, 2019

fixes #49

@dnsl48 dnsl48 changed the base branch from master to 2.1 June 28, 2019 03:07
class OpenSSLCryptoTest extends SapphireTest
{
/**
* @requires extension openssl
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL 👍

* Silverstripe 4.4 does not have a binary database field implementation, so we have to store
* binary data as text.
*
* @internal This class is internal API of DatabaseStore and may change without warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really a fan of doing this to be honest. How about we try and do this instead?

  • Define a CodecInterface
  • Add a DataCodec implementation of it (this class) - maybe call it JsonDataCodec?
  • Configure it as the default implementation in YAML config
  • Use Injector DI to inject the CodecInterface to the stores that need it

If we have to change something that can't be done in the bounds of semver, then we can create a new implementation and switch the config to use the new one - this can be done in a patch release, then we can deprecate the old one in a minor release, and we won't need internal API tags.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in person.

  • DataCodec becomes a trait JsonDataCodec
  • DatabaseStore uses the JsonDataCodec as a part of its implementation. There's not much value in having the codec registered as a service in the Injector. However, a trait is reusable enough for custom database store implementations

The above happens for the next minor release.

For the patch release I'm going to move DataCodec methods into the DatabaseStore class as private methods (internal implementation details)

@dnsl48
Copy link
Contributor Author

dnsl48 commented Jun 28, 2019

Thanks for the review. Going to rewrite those bits as discussed.

@dnsl48 dnsl48 closed this Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants