Skip to content

Conversation

@Tobion
Copy link
Contributor

@Tobion Tobion commented Oct 8, 2015

  1. fix delete() return value when file does not exist: it must return true even if the cache entry doesn't exist. False should only be returned in case of a failed delete. Since the outcome of deleting a non-existing entry is what we wanted anyway (the entry doesn't exist anymore), it should return true. But the filesystem cache returned false because unlink returns false if the file does not exist. This was also broken for some other cache drivers.
  2. fix flushAll() and getStats() for file cache to ignore dot files and clean up balancing directories: before it also iterated over dot files . and .. (if the extension is empty) trying to delete them, which of course failed, but was silenced. and intermediate directories for balancing were never removed. flushAll only removed the files but not the directories leaving alot of garbage behind.
  3. tests were added for the above things and tests for the existing behavior that caches in a shared directory but different extensions do not influence each other.
  4. fix Memcached handling of false as data

@Ocramius Ocramius added the Bug label Oct 8, 2015
@Ocramius Ocramius self-assigned this Oct 8, 2015
@Ocramius Ocramius added this to the 1.4.2 milestone Oct 8, 2015
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the cleanup was missing to remove the cache root dir thus every test run would create a new temp dir without deleting it.

@Tobion
Copy link
Contributor Author

Tobion commented Oct 8, 2015

Some cache providers fail the testDeleteIsSuccessfulWhenKeyDoesNotExist but others succeed like the ArrayCache. So there are some incosistencies in the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is indeed painful :|

@Tobion
Copy link
Contributor Author

Tobion commented Oct 8, 2015

@Ocramius I fixed all drivers that were failing for deleting non-existing entries. The cache suit passes now.

@Tobion Tobion changed the title Fix delete() and flushAll() for filesystem cache Fix delete() and flushAll() Oct 8, 2015
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test ignored the data provider as $value has not been used at all.

Ocramius added a commit that referenced this pull request Oct 28, 2015
@Ocramius Ocramius merged commit 58752b0 into doctrine:master Oct 28, 2015
@Ocramius
Copy link
Member

Merged and backported into 1.4.x via cdeb14a, thanks!

@Tobion Tobion deleted the fix-delete-filesystem branch October 28, 2015 11:05
@Ocramius Ocramius modified the milestones: 1.4.3, 1.4.2 Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants