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 #25 #26

Merged
merged 4 commits into from
Dec 1, 2021
Merged

fix #25 #26

merged 4 commits into from
Dec 1, 2021

Conversation

fezfez
Copy link
Contributor

@fezfez fezfez commented Nov 30, 2021

No description provided.

Bumps [laminas/laminas-cache](https://github.com/laminas/laminas-cache) from 3.0.x-dev to 3.1.2.
- [Release notes](https://github.com/laminas/laminas-cache/releases)
- [Commits](https://github.com/laminas/laminas-cache/commits/3.1.2)

---
updated-dependencies:
- dependency-name: laminas/laminas-cache
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@boesing
Copy link
Member

boesing commented Nov 30, 2021

Thanks for this PR.
Could you also add a failing unit test so we can prevent this error for future development?
Thanks in advance.

Would also be great if you rebase against 2.0.x branch (I can do that as well but if you are working on it, that would be great).

@boesing boesing added the Bug Something isn't working label Nov 30, 2021
@boesing boesing added this to the 2.0.1 milestone Nov 30, 2021
@boesing boesing changed the base branch from 2.1.x to 2.0.x November 30, 2021 17:36
@boesing boesing linked an issue Nov 30, 2021 that may be closed by this pull request
dependabot bot and others added 2 commits December 1, 2021 06:47
Bumps [vimeo/psalm](https://github.com/vimeo/psalm) from 4.12.0 to 4.13.0.
- [Release notes](https://github.com/vimeo/psalm/releases)
- [Commits](vimeo/psalm@4.12.0...4.13.0)

---
updated-dependencies:
- dependency-name: vimeo/psalm
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Stéphane Demonchaux <demonchaux.stephane@gmail.com>
@fezfez
Copy link
Contributor Author

fezfez commented Dec 1, 2021

@boesing : done

Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM.

I must admit, that I am not even aware of the functionality of this code. We are detaching a listener from the event handler, which is always null.
So there is absolutely no chance that this code has any impact.

I wonder if this could be removed.

When looking into the previous implementation, this was related to the Filesystem#getTotalSpace method and was to reset an already detected, cached, total space value for a preconfigured directory. Not sure if the detach logic provides anything useful - but this is just for the sake of completeness here. I will create an issue from this comment for further investigation and refactoring.

@boesing
Copy link
Member

boesing commented Dec 1, 2021

Thanks, @fezfez!

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.

Not working with laminas/laminas-eventmanager:3.4
2 participants