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

CallbackCache: simplify calling function/method #63

Merged
merged 2 commits into from May 16, 2021
Merged

CallbackCache: simplify calling function/method #63

merged 2 commits into from May 16, 2021

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Jan 26, 2021

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

Description

Simplified calling the underlying function/method of CallbackCache (and ObjectCache and ClassCache)

@boesing boesing added this to the 3.0.0 milestone Jan 28, 2021
@boesing
Copy link
Member

boesing commented Jan 28, 2021

LGTM so far.
Thanks for contributing this!
I'll merge this when I've implemented GHA for CI.
For more details, see laminas/technical-steering-committee#61

@boesing boesing changed the base branch from 3.0.x to 2.12.x May 8, 2021 14:14
@boesing boesing changed the base branch from 2.12.x to 3.0.x May 8, 2021 14:26
@boesing
Copy link
Member

boesing commented May 8, 2021

Hm, I was wondering if I could backport this to v2 but actually, this is a BC break.
I've implemented GHA already and it turns out that changing the callback to [$className, $method] changes its type from string to array and thus, leads to an error when the callback key is generated.

https://github.com/laminas/laminas-cache/pull/63/files#diff-affcf97709fa42da6b51246d0e1d19dfbd80479d3cfbf2c35eecae7358107e6dR63

$callbackKey = md5(strtolower($callback));

I will rebase this PR against 3.0.x as it contains GHA so we can see the failing tests in here.

@boesing
Copy link
Member

boesing commented May 8, 2021

Wait what? I've tried to rebase against 2.12.x but that led into failing tests.
Why the heck isn't that the case in 3.0.x - I am confused...


Okay, somehow the names of the test classes changed due to some of the changes made back in february. I've fixed that with #103 and rebased against 3.0.x now.

@marc-mabe
Copy link
Contributor Author

@boesing should be fine now

@boesing
Copy link
Member

boesing commented May 12, 2021

Thanks, Marc.
Looks good. As far as I can see, this wont be a BC break, right?
I'll probably retarget this against 2.12.x if its not considered BC break so we get this out of the door somewhen in the next weeks rather than months.
WDYT?

@marc-mabe
Copy link
Contributor Author

I don't see any bc break here. It's just an internal optimisation.

Signed-off-by: Marc Bennewitz <m.bennewitz@dcmn.com>
Signed-off-by: Marc Bennewitz <m.bennewitz@dcmn.com>
@boesing boesing changed the base branch from 3.0.x to 2.12.x May 16, 2021 14:59
@boesing boesing modified the milestones: 3.0.0, 2.12.0 May 16, 2021
@boesing
Copy link
Member

boesing commented May 16, 2021

GHA have major outage as of https://www.githubstatus.com/incidents/zbpwygxwb3gw

Verified functionality locally on all three PHP versions.
Thanks, @marc-mabe!

@boesing boesing merged commit efacd6d into laminas:2.12.x May 16, 2021
@marc-mabe marc-mabe deleted the args branch May 17, 2021 06:16
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

3 participants