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

[#1878] User definable duplication for DeduplicationHandler #1879

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

cracksalad
Copy link
Contributor

This is a draft to solve #1878

Feel free to comment on the changes I made!

Although the tests ran through without modification, this change is not backwards compatible when you look at it closely, because it changes the signature of several methods (changed visibility from private to protected, add parameters etc.).

Comment on lines 77 to 83
$yesterday = time() - 86400;
for ($i = 0; $i < count($store); $i++) {
if (substr($store[$i], 0, 10) < $yesterday) {
$gc = true;
break;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I would rather keep the gc property and have isDuplicate set it to true if an older entry is found, because the way this works here, if you override the file format in buildDeduplicationStoreEntry/isDuplicate, then you may break the GC functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point! I moved the GC trigger back to isDuplicate(), readded the property and added a comment to isDuplicate() which hopefully explains to overwriters that they should set $this->gc.

I hope the failing CI workflow is not a huge problem. I am not sure what to do about it, because my changes do not seem to be responsible.

Copy link
Owner

Choose a reason for hiding this comment

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

Nope there's a mess on CI right now, I'm on it :)

@Seldaek Seldaek added this to the 3.x milestone Apr 12, 2024
@cracksalad cracksalad marked this pull request as ready for review April 12, 2024 12:19
Copy link
Owner

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, found a couple nitpicks still

src/Monolog/Handler/DeduplicationHandler.php Outdated Show resolved Hide resolved
src/Monolog/Handler/DeduplicationHandler.php Outdated Show resolved Hide resolved
src/Monolog/Handler/DeduplicationHandler.php Outdated Show resolved Hide resolved
@Seldaek Seldaek merged commit c48c642 into Seldaek:main Apr 12, 2024
4 of 9 checks passed
@Seldaek
Copy link
Owner

Seldaek commented Apr 12, 2024

Thanks!

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.

None yet

2 participants