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

Add missing template param for StorageInterface #319

Conversation

boesing
Copy link
Member

@boesing boesing commented Jun 21, 2024

Q A
Bugfix yes
BC Break soft
New Feature no

Description

Since all laminas cache adapters are extending AbstractStorage, there was already a template TOptions available in that abstract class. Sadly, I forgot to add that to the StorageInterface which is problematic when it comes to interface usage.

This patch adds options template as a bugfix since we do not change any code (besides providing the options generic).

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing added the Bug Something isn't working label Jun 21, 2024
@boesing boesing added this to the 4.0.1 milestone Jun 21, 2024
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

👍 - Out of interest, why do the templates not need to be co-variant?

@boesing
Copy link
Member Author

boesing commented Jun 21, 2024

Out of interest, why do the templates not need to be co-variant?

I have absolutely no clue when it has to be and when not.
I've used covariant once but I cannot remember when. AFAIR covariant is only used if you want to mix stuff such as "pets" where "dog" and "cat" has to be in there.
Storage adapters only provide a single generic and thus covariant is not needed as from how I understand.

But I am not that deep into that stuff as I never really cared 😅

@boesing boesing merged commit 03b73c8 into laminas:4.0.x Jun 21, 2024
18 checks passed
@boesing boesing deleted the bugfix/add-missing-template-type-storageinterface branch June 21, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants