Skip to content

Conversation

@rrooij
Copy link
Contributor

@rrooij rrooij commented Apr 7, 2018

This makes it easier to generate getters and setters for class
attributes in PHP.

@AndreaCrotti
Copy link
Owner

I'm not a PHP expert but from a quick search it doesn't look like it's such common practice to implement it this way.
I saw some examples of __get and __set but never just get and set, any resources to point out this is kind of standard?

The other thing I don't like too much is that the order becomes

  • var declaration
  • getter
  • setter

While maybe you might want a different layout like adding all the variables first.
Thanks for your contribution anyway of course, but unless it's something quite generic that more or less everyone could use I'd rather not merge this one..

@AndreaCrotti AndreaCrotti added the seen PR was seen and initial feedback was given label Apr 7, 2018
@rrooij
Copy link
Contributor Author

rrooij commented Apr 7, 2018

Thanks for the feedback.

It is a common practice in PHP frameworks like Symfony. See:
https://symfony.com/doc/current/doctrine.html#generating-getters-and-setters

Using the magic get and set methods is discouraged, see:

https://stackoverflow.com/questions/6184337/best-practice-php-magic-methods-set-and-get/6184893#6184893

It is common in OOP PHP projects.

I understand your concern regarding the layout. Feel free to decide what you want to do with it.

@AndreaCrotti
Copy link
Owner

Ok I see thanks for the extra information.
The simplest solution is just to add two separate snippets for the getter and the setter, which could be triggered by set and get for example?
In that way you would not really have any issues with the layout anymore, and allow people to use these snippets more freely.

@rrooij
Copy link
Contributor Author

rrooij commented Apr 7, 2018

That is a good solution indeed. Should I rebase in this pull request and squash it with the commit?

Edit: Done

@rrooij rrooij force-pushed the php_class_attribute branch from 91f67c9 to 819de8b Compare April 7, 2018 17:03
@rrooij rrooij force-pushed the php_class_attribute branch from 819de8b to dce86f7 Compare April 7, 2018 17:24
@AndreaCrotti
Copy link
Owner

Yes ok looks good thanks

@AndreaCrotti AndreaCrotti merged commit 4bbe565 into AndreaCrotti:master Apr 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

seen PR was seen and initial feedback was given

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants