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

all datatypes for blackhole adapter #26

Closed
wants to merge 1 commit into from

Conversation

JesBei
Copy link

@JesBei JesBei commented Jul 16, 2020

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

...could not be created. Reason: The storage adapter "Zend\Cache\Storage\Adapter\BlackHole" requires a serializer plugin; please see https://docs.zendframework.com/zend-cache/storage/plugin/#quick-start for details on how to attach the plugin to your adapter.
... could not be created. Reason: The adapter 'Zend\Cache\Storage\Adapter\BlackHole' doesn't implement 'Zend\EventManager\EventsCapableInterface' and therefore can't handle plugins

Blackhole adapter now accepts all datatypes and doesn't need a serializer anymore. Otherwise you have to implement the EventSystem but I don't think that's necessary because blackhole doesn't save anything anyway.

@michalbundyra michalbundyra added Bug Something isn't working Unit Test Needed labels Jul 16, 2020
Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@JesBei Thanks for your contribution.

In order to accept the PR we need unit test to be added to cover the changes.
Also please revert all unrelated formatting changes and check DCO status. Thanks.

@fabiang
Copy link

fabiang commented Jul 16, 2020

Btw: the error described by @JesBei happens when using the PSR-16 decorator, which leaves the Blackhole adapter unusable together with PSR-16.

Signed-off-by: JesBei <jesper.beisner@yahoo.de>
@JesBei
Copy link
Author

JesBei commented Jul 16, 2020

@JesBei Thanks for your contribution.

In order to accept the PR we need unit test to be added to cover the changes.
Also please revert all unrelated formatting changes and check DCO status. Thanks.

Added the Unit Test and reverted the formatting.

@boesing
Copy link
Member

boesing commented Jul 20, 2020

@JesBei Could you please add integration tests for Blackhole aswell?
There are several integration tests in the tests/Psr directory, you can just copy & paste one of these and apply changes to use the Blackhole adapter.

As @fabiang states, there is an issue with the PSR decorators. Thus, with those tests, we would have initial failing tests aswell.

I will move those changes to laminas/laminas-cache-storage-adapter-blackhole in case we are fixing that "issue" in a patch version. If not, these changes will definitely be part of the 2.10 milestone.

@fabiang
Copy link

fabiang commented Aug 25, 2020

@JesBei Could you please add integration tests for Blackhole aswell?
There are several integration tests in the tests/Psr directory, you can just copy & paste one of these and apply changes to use the Blackhole adapter.

The integration tests does make much sense, since the blackhole adapter doesn't cache anything and as a result the tests that come from https://github.com/php-cache/integration-tests should fail. The only reasonable test could be that the decorators can handle the adapter and imho this could be done in the unit test directly.

@boesing
Copy link
Member

boesing commented Sep 15, 2020

@JesBei I have to revert my latest statement, that this will be definetely be part of the next minor (2.10.0).
The next minor version (2.10.0) of laminas-cache will introduce satellite packages for all storage adapters. Thus, I have to close PRs in here which point to adapters.

If you still need support for this feature, please fork laminas/laminas-cache-storage-adapter-blackhole and re-open a PR in the adapter project.

@boesing boesing closed this Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Unit Test Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants