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

Replace Model::persistence property with getter/setter #948

Merged
merged 7 commits into from
Jun 29, 2022

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jan 1, 2022

previously Persistence::add() was not called when Model::persistence was set and it implied no Persistence::HOOK_AFTER_ADD hook was called

this PR fixes it by replacing Model::persistence property with persistence getter/setter

with the newly introduced persitence Model::setPersistence() setter, Persistence:add() call does not make any logical sense ( persistence can be set only once and never unset) and thus direct Persistence:add() call newly throws an exception

Here are several examples how to upgrade your code when setting persistence:

regex to locate old usage: (?<!\w)(db|persistence)(?!\w).*->add\(

 $m = new Invoice($db);
 // no change required
-$m = $db->add(new Invoice());
+$m = new Invoice($db);
 $m = new Invoice();
 ...
-$db->add($m);
+$m->setPersistence($db);

When accessing the persistence, Model::getPersistence() getter must be used and it throws when persistence is not set. To check if persistence is set, use Model::issetPersistence() method.

regex to locate old usage: ->persistence(?!\w)

-$m = new Invoice($this->persistence);
+$m = new Invoice($this->getPersistence());

@mvorisek mvorisek added the bug label Jan 1, 2022
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from a2ca9fb to d263710 Compare January 1, 2022 21:22
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from d263710 to 3bdbd6c Compare January 1, 2022 21:26
@mvorisek mvorisek marked this pull request as draft January 1, 2022 21:51
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from 3bdbd6c to 4c65e89 Compare June 25, 2022 08:37
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from 4c65e89 to 1d415f8 Compare June 28, 2022 10:58
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch 2 times, most recently from af17abf to 6bf16d4 Compare June 28, 2022 11:28
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from 6bf16d4 to 34202ef Compare June 28, 2022 11:39
@mvorisek mvorisek changed the title Persistence::add() must be called when Model::persistence is set Replace Model::persistence property with getter/setter Jun 28, 2022
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from fd828eb to 5727b8d Compare June 29, 2022 10:02
@mvorisek mvorisek force-pushed the persistence_add_called_only_from_constructor branch from 5727b8d to 77d50e8 Compare June 29, 2022 10:13
@mvorisek mvorisek marked this pull request as ready for review June 29, 2022 10:13
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

Looks good. Have to fix tests.

@mvorisek
Copy link
Member Author

what tests you mean?

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

Successfully merging this pull request may close these issues.

2 participants