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

Rule: MEQP2.Classes.ConstructorOperations #46

Open
schmengler opened this issue Jul 12, 2018 · 4 comments
Open

Rule: MEQP2.Classes.ConstructorOperations #46

schmengler opened this issue Jul 12, 2018 · 4 comments
Assignees

Comments

@schmengler
Copy link
Collaborator

Rule

Only dependency assignment operations are allowed in constructor. No other operations are allowed.

The rule can be included from MEQP2 with severity 5 (default), and needs to be documented with examples.

Reason

Magento instantiates all objects in the dependency graph early (if no proxy is used), so instantiation must 1. be fast and 2. not depend on any application state

@jissereitsma
Copy link
Contributor

See #54

@jissereitsma jissereitsma self-assigned this Jul 24, 2018
@schmengler schmengler added on agenda of hangout This issue will be discussed in the next open hangout and removed debate needed on agenda of hangout This issue will be discussed in the next open hangout labels Jul 26, 2018
@schmengler
Copy link
Collaborator Author

To do: add a simple example

@davidverholen
Copy link

@schmengler I totally agree, but(!) what about classes, that are not necessarily instantiated via the OM?

Value Objects for example should validate the input in the constructor (from what I've learned last year in amersfoort ;) ).

I think there is already a very similar discussion in the "when to use new" issue #17

@jissereitsma
Copy link
Contributor

I complete agree with @davidverholen . Classes like Value Objects should impose stricter rules upon constructor arguments than the ones supplied by PHP 7 (using type hinting with Strict Types enabled). Simple types (double $moneyAmount) are best converted into Value Objects (Money $moneyAmount) but still those Value Objects (class Money) should check whether their own arguments (double $amount) are valid - and in the example of this sentence, a double is not enough: The value could be zero (while maybe this makes less sense) or negative (which makes perhaps no sense).

But perhaps, we can negotiate the meaning of dependency assignment operations as the rule definition mentions it. Assuming that the variables $foo and $bar are injected through the constructor, I would say that the following needs to be taken into account:

  • $this->foo = $foo and $this->bar = $bar are perfectly fine.
  • $this->setFoo($foo) is essentially the same as $this->foo = $foo, except that using a separate method for this allows for better input validation (where PHP type hinting is not enough).
  • $this->foobar = $foo + $bar is still an assignment, but I would say this is more configuration than assignment and it should not be allowed.

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

No branches or pull requests

3 participants