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

Deprecate PatternOptions storage option #98

Merged
merged 7 commits into from Jul 11, 2021

Conversation

boesing
Copy link
Member

@boesing boesing commented May 2, 2021

Q A
Documentation yes
BC Break no
New Feature yes

Description

Since v2.10, the storage adapter have their own package. In v3.0 these, adapters have to register themselves to the AdapterPluginManager when the plugin manager is being instantiated and thus, this component wont be aware of which adapters are available without using a PSR-11 container.

This commit aims to provide a proper dependency injection layer to all cache patterns while deprecating the old storage option for PatternOptions.

Closes #97

$this->options = $options;
return $this;
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

These are part of the PatternInterface

/**
* Set pattern options
*
* @param PatternOptions $options
Copy link
Member Author

Choose a reason for hiding this comment

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

These are part of the PatternInterface

*/
protected $storage;

public function __construct(?StorageInterface $storage = null, ?PatternOptions $options = null)
Copy link
Member Author

Choose a reason for hiding this comment

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

Will be changed to required arguments in v3.0

@boesing boesing force-pushed the qa/pattern-options-storage-option branch 2 times, most recently from 77969fa to 680478b Compare May 2, 2021 19:30
@boesing boesing requested a review from froschdesign May 2, 2021 19:32
@boesing
Copy link
Member Author

boesing commented May 2, 2021

@froschdesign I am not 100% sure but there are markdown issues with the tables.

In the documentation HTML, it looks correct but using pipes within backticks while in a table column leads to display problems in non-HTML markdown editors.

So this is actually problematic:

Option | Data Type | Default Value | Description
------ | --------- | ------------- | -----------
`storage` | `string | array | Laminas\Cache\Storage\StorageInterface` | none | Adapter used for reading and writing cached data.

https://gist.github.com/boesing/c37618ee975af0accbec2a6ce07cb458

It seems that this should be used instead (but leads to markdownlint issues due to the code usage):

Option | Data Type | Default Value | Description
------ | --------- | ------------- | -----------
`storage` | <code>string &#124; array &#124; Laminas\Cache\Storage\StorageInterface</code> | none | Adapter used for reading and writing cached data.

https://gist.github.com/boesing/44e1ad9d9dd424d01b6e6393d50a4387

@froschdesign
Copy link
Member

We had the same problem in laminas-navigation: https://github.com/laminas/laminas-navigation/blob/2.12.x/docs/book/pages.md#common-page-options

Add a backslash:

Option | Data Type | Default Value | Description
------ | --------- | ------------- | -----------
`storage` | `string\|array\|Laminas\Cache\Storage\StorageInterface` | none | Adapter used for reading and writing cached data.
Option Data Type Default Value Description
storage string|array|Laminas\Cache\Storage\StorageInterface none Adapter used for reading and writing cached data.

@boesing boesing requested a review from weierophinney May 8, 2021 13:58
@boesing boesing force-pushed the qa/pattern-options-storage-option branch from 44a64ae to 742cf7c Compare May 12, 2021 18:22
'cache_output' => true,
]);
/** @var StorageInterface $storage */
$storage = null; // Can be any instance of StorageInterface
Copy link
Member

Choose a reason for hiding this comment

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

The pattern section contains an introduction page, it would be good if a standard storage adapter for the documentation of the cache patterns is defined there. This can be done in blockquote and can then be linked.

For laminas-i18n I used the filesystem adapter: https://docs.laminas.dev/laminas-i18n/translator/caching/

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I've tried something but not sure if thats what you had in mind. Feel free to review.

@boesing boesing requested a review from froschdesign May 16, 2021 14:37
@boesing boesing force-pushed the qa/pattern-options-storage-option branch 2 times, most recently from 8cebdfa to fd840ba Compare May 16, 2021 18:14
@boesing boesing force-pushed the qa/pattern-options-storage-option branch from fd840ba to 266144b Compare May 24, 2021 21:07
@boesing
Copy link
Member Author

boesing commented May 24, 2021

@froschdesign If you have some time, I'd love to get feedback on my attempt to provide a Standard Storage Adapter documentation.

docs/book/pattern/callback-cache.md Show resolved Hide resolved
docs/book/pattern/capture-cache.md Outdated Show resolved Hide resolved
docs/book/pattern/intro.md Outdated Show resolved Hide resolved
docs/book/pattern/output-cache.md Outdated Show resolved Hide resolved
docs/book/pattern/class-cache.md Outdated Show resolved Hide resolved
@froschdesign
Copy link
Member

@boesing
Sorry for the very late response. Please ping me, if you need help!

@boesing boesing requested a review from froschdesign June 6, 2021 12:01
@boesing boesing force-pushed the qa/pattern-options-storage-option branch from 6ea067b to 1a9caf6 Compare July 11, 2021 11:35
Since v2.10, the storage adapter have their own package. In v3.0 these, adapters have to register themselves to the `AdapterPluginManager` when the plugin manager is being instantiated and thus, this component wont be aware of which adapters are available without using a `PSR-11` container.

This commit aims to provide a proper dependency injection layer to all cache patterns while deprecating the old `storage` option for `PatternOptions`.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
…ck start section

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
…ipts

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
- replaced dead links from example with github release notes
- imported laminas-feed `Reader` class

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member Author

boesing commented Jul 11, 2021

@froschdesign I'll merge this for now. We can optimize documentation in dedicated branches. I'd rather go moving forward with the release of v2 so we can finally kick out v3.

@boesing boesing merged commit 93ea2fe into laminas:2.12.x Jul 11, 2021
@boesing boesing deleted the qa/pattern-options-storage-option branch July 11, 2021 11:43
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