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 incorrect function calls in Units #3088

Closed
wants to merge 1 commit into from
Closed

Conversation

tben
Copy link

@tben tben commented Oct 11, 2024

The CarbonInterval make function appears to accept the following arguments: $intervals, $units, and $skipCopy, but Units class is instead sending $units, an empty array, and true. This causes issues with the following calls ->sub('day') ->add('day') or ->subtract('day') on the Carbon object

Useful Link: https://github.com/briannesbitt/Carbon/blob/5607f63713142842b3cac9c9a69b371bc27f213b/src/Carbon/CarbonInterval.php#L1201C16-L1201C88

@tben tben changed the title Fix Incorrect Arguments for CarbonInterval Fix incorrect function calls in Units Oct 11, 2024
@kylekatarnls
Copy link
Collaborator

Hello,

What issue does it causes? As you can see with the unit tests, it breaks badly multiple features. I suggest you first open an issue with a description of the bug you're facing, how to reproduce and what behavior you were expecting.

We also expect merge request to come with a unit test showing what problem they are solving and then ensuring it would not be broken by a future change.

@tben
Copy link
Author

tben commented Oct 11, 2024

Thanks for the quick response. I think I've misunderstand how this function works. My problem was that I'm trying to do Carbon::parse('now')->sub('day'); which in my mind would take away a day but just result in an exception. I was debugging it on my local vm and swapping these variables fixed it but I clearly didn't account for when ->sub('1 day') is used. Closing pull request since this isn't a good enough solution.

@tben tben closed this Oct 11, 2024
@tben tben deleted the patch-1 branch October 11, 2024 20:03
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.

2 participants