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

Add get/set support for DIContainerTrait::setDefaults() #245

Closed
wants to merge 4 commits into from

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jun 12, 2020

fixes #206 , needed for #227

done, but need tests

maybe even add/remove with syntax like class.adds = ['x', 'y'], ...

@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch 2 times, most recently from 236fe52 to aae0b02 Compare June 12, 2020 23:06
@mvorisek mvorisek requested a review from DarkSide666 June 12, 2020 23:13
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch 2 times, most recently from fc1ba36 to fc0e208 Compare June 13, 2020 12:38
@atk4 atk4 deleted a comment from codecov bot Jun 14, 2020
@DarkSide666
Copy link
Member

Not sure about this.
@abbadon1334 comment #206 (comment) sounds quite legit.

@georgehristov
Copy link
Collaborator

I am "for" it and was even proposing it in the chat some time ago.

@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch 2 times, most recently from e1aa2c9 to ada20a4 Compare July 8, 2020 09:04
@mvorisek mvorisek marked this pull request as draft August 25, 2020 08:57
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from ada20a4 to 5bf9d5d Compare August 26, 2020 10:51
@mvorisek mvorisek requested a review from georgehristov August 26, 2020 11:19
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from 33e9087 to 79a7963 Compare August 26, 2020 11:22
@mvorisek
Copy link
Member Author

mvorisek commented Aug 26, 2020

Not sure about this.
@abbadon1334 comment #206 (comment) sounds quite legit.

we can implement it using 2pass approach, ie.

  1. set DIRECTLY ALL values FIRST (if declared or detected to be magical)
  2. then for each value (if setter exists):
    a) reset to old value
    b) set using invoking setXxx()

@abbadon1334 does this address your comment?

@DarkSide666 @georgehristov wdyt, any better idea?

@mvorisek
Copy link
Member Author

mvorisek commented Aug 26, 2020

another option might be to use property anotations for getter/setter method names, but looks like quite a lot horsepower for the current state of this project now

@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from f652977 to 1b890e2 Compare August 26, 2020 19:01
$isMissing = true;

try {
$origValue = $this->{$name} ?? null;
Copy link
Member Author

Choose a reason for hiding this comment

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

this will still not work if null...

@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from d4304aa to e85b785 Compare June 13, 2023 15:42
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from e1b8e2e to 7cd9e76 Compare July 16, 2023 14:51
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch 2 times, most recently from a3e0eec to 7ac2e7e Compare October 1, 2023 13:46
@mvorisek mvorisek force-pushed the set_defaults_with_setXxx branch from 7ac2e7e to 739bb7a Compare March 31, 2024 21:51
@mvorisek
Copy link
Member Author

closing in favor of https://wiki.php.net/rfc/property-hooks

@mvorisek mvorisek closed this Mar 31, 2024
@mvorisek mvorisek deleted the set_defaults_with_setXxx branch March 31, 2024 22:37
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.

Allow/prioritize setter methods in DIContainerTrait::setDefaults()
3 participants