Avoid calling query() twice by adding a local var, fixes #4133 and #4140 #4221
Conversation
|
Uhhh, failing tests... When was |
|
Is the clone necessary? |
|
It seems to be – if you remove it, the test fails because the query has the chunking clause attached to it, so returns a smaller number of items than the full query, so the results don't match. Using a clone means the original is left intact. |
|
Perhaps we can do a check if the clone method exists. If not we just call the entire query method again so it has the current behavior. Support for older Laravel versions will be dropped anyway in the near future so I don't mind if the issue remains for the legacy versions. |
|
Instead of that, I used |
|
As I mentioned in the original PR description, it's quite likely that this problem exists in other places too; we've just not run into them yet |
1️⃣ Why should it be added? What are the benefits of this change?
This is a fix for an issue reported in the Laravel Nova Excel project - though it turned out that the issue was here, not there.
The problem is that if query() makes non-idempotent changes to the $query, it's likely to result in a corrupt query. So to avoid this, we need to make sure we only call query() once, and once we have an instance, take care not to alter it further. I found that calling chunk() on $query altered it, making a test fail. I solved that by cloning it, so the changes caused by chunk can be discarded.
2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No, just one.
3️⃣ Does it include tests, if possible?
Yes
4️⃣ Any drawbacks? Possible breaking changes?
If there are other places that
query()is called in a similar way, the same problem may occur there too. I ran into the same thing inside a test.5️⃣ Mark the following tasks as done:
6️⃣ Thanks for contributing! 🙌