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

Move "app" and "owner" props to getter/setter #277

Merged
merged 7 commits into from
Oct 6, 2020
Merged

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Oct 1, 2020

there are at least 3 reasons why:

  • have more control (type check, reassign, enforce presence in some situations like after init, ...)
  • allow to use WeakReference (for owner to allow to release child object based on ref counting not requiring expensive/slow GC)
  • make sure we do not get/set properties on object not "implementing" given traits

if AppScopeTrait trait is present, we may even want to assert if App is set in any init

How to update your code?

  1. replace ->owner-> with ->getOwner()->
  2. search for ->owner(?!\w) and adjust to getOwner(), setOwner(), issetOwner() manually
  3. do the same with app property

*/
public $app;
private $app;
Copy link
Member Author

Choose a reason for hiding this comment

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

because of trait design, private prop will still be accessible from the class where used

we may want to rename to _app, but then any assigment will not throw - but it can be solved by checking ->app inside init or in the getter/setter methods in this trait

return $this->_app !== null;
}

public function getApp(): App
Copy link
Member Author

Choose a reason for hiding this comment

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

impossible to test if there is no App class - do we want this trait really in core? If yes, we can enforce object type and phpdoc App.

@mvorisek mvorisek force-pushed the getter_app branch 2 times, most recently from 255ca8c to d73c02b Compare October 2, 2020 11:40
@mvorisek mvorisek force-pushed the getter_app branch 2 times, most recently from be7e611 to fdbb807 Compare October 2, 2020 11:57
@mvorisek mvorisek changed the title WIP App prop to getter/setter WIP "app" and "owner" props to getter/setter Oct 5, 2020
@mvorisek mvorisek changed the title WIP "app" and "owner" props to getter/setter Move "app" and "owner" props to getter/setter Oct 5, 2020
@mvorisek mvorisek marked this pull request as ready for review October 6, 2020 05:37
@mvorisek
Copy link
Member Author

mvorisek commented Oct 6, 2020

@georgehristov PRs for all major repos are done, ok to merge?

@romaninsh
Copy link
Member

can this be backward compatible for 1 major release?

@mvorisek
Copy link
Member Author

mvorisek commented Oct 6, 2020

can this be backward compatible for 1 major release?

no, as it would require magic properties almost in every object

the good new is, that existing code can be migrated almost completely with "ctrl+h"

@romaninsh
Copy link
Member

@mvorisek I'd love if we avoid this. I mean you could add a dummy getOwner() that returns $this->owner for entire release for backward compatibility. Can we have that for 2.3 so that users could start updating code before migrating?

Copy link
Member

@romaninsh romaninsh left a comment

Choose a reason for hiding this comment

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

OK, but please add compatibility into 2.3 release branch.

@mvorisek mvorisek merged commit 88412f2 into develop Oct 6, 2020
@mvorisek mvorisek deleted the getter_app branch October 6, 2020 08:41
mvorisek added a commit that referenced this pull request Apr 5, 2021
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