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

[10.x] Allow object caching to be disabled for custom class casters #47423

Merged
merged 2 commits into from
Jun 18, 2023

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Jun 13, 2023

Hello!

This is in a similar vein as #40519, but for custom class casters. I need to disable object caching but there's currently no way to do this with custom casters. This PR allows a boolean withoutObjectCaching property to be defined on the class casters which HasAttributes then checks for before caching.

Thanks!

@taylorotwell taylorotwell merged commit 783a000 into laravel:10.x Jun 18, 2023
16 checks passed
@calebdw calebdw deleted the class_casters branch June 18, 2023 20:24
@JamShady
Copy link
Contributor

JamShady commented Jul 2, 2023

I realise I'm a little late to the discussion on this, but was/is there a reason this property has to be named 'withoutObjectCaching' (i.e. negative) that defaults to false, rather than just something like 'cacheObject' which defaults to true?

In my mind, I've got to do unnecessary mental gymnastics in thinking "I want object caching, therefore my desired value is true, but the variable is in the negative, so I have to flip my desired value around and set to false", as opposed to the other way which would be "I don't want object caching, so I just set the value to false" which is far less neural load and less likely to be set incorrectly, resulting in less potential developer frustration when it turns out they accidentally set the wrong value due to this confusion.

I hope that makes sense.

Sorry to tag you @taylorotwell I wasn't sure if you'd get notified otherwise. I'm happy to put in a PR for this if you deem it a good idea?

@calebdw
Copy link
Contributor Author

calebdw commented Jul 2, 2023

@JamShady,

Yes there were reasons:

  1. It follows the same public api as ->withoutObjectCaching() for mutators
  2. The default behaviour is object caching and can't force everyone to have to add the property to their class in order for default behaviour to continue working (breaking change)
  3. Not a fan of a missing property defaulting to true, a missing property should be considered "falsey" in my opinion
  4. There's no reason to ever specify $withoutObjectCaching = false if you want the default behaviour, you should only add the property to the class if you want to disable caching in which case $withoutObjectCaching = true reads better (the boolean is really just for completeness, code could have just checked for property existence).

It's pretty clear to me what the property does and doesn't require a lot of "mental gymnastics" to understand that the value should be set to true if you want to disable object caching...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants