Remove setters from UserInterface #367

texdc · 2013-11-05T22:39:56Z

Setter methods are potentially unnecessary and may vary per implementation due to PHP's limitations on type-hinting and method signatures. For instance, if I have an EmailAddress value object, I'll want to construct my user entity like so:

// ...
public function setEmail(EmailAddress $email)
{
    $this->email = (string) $email;
}
// ...

A fatal error occurs:

Fatal error: Declaration of My\Entity\UserTrait::setEmail(EmailAddress $email) must be compatible with ZfcUser\Entity\UserInterface::setEmail($email)

Also, if visibility is changed on the setters, fatal errors occur. In DDD entities, the $id is immutable:

Fatal error: Access level to My\Entity\UserTrait::setId() must be public (as in class ZfcUser\Entity\User)

JanMalte · 2013-11-20T13:50:57Z

👎
I disagree. I think setter must stay in the interface. If you introduce an EmailAdress object, you have to cast it to string befor giving it as a parameter to the setter. You don't make the interface more and more flexible if you just remove everything.

texdc · 2013-11-20T15:19:01Z

Wrong. By defining a method in an interface, you're limiting the implementation. Remove the definition to add flexibility and extensibility.

On Nov 20, 2013, at 7:51 AM, Jan Malte Gerth notifications@github.com wrote:

I disagree. I think setter must stay in the interface. If you introduce an EmailAdress object, you have to cast it to string befor giving it as a parameter to the setter. You don't make the interface more and more flexible if you just remove everything.

—
Reply to this email directly or view it on GitHub.

danizord · 2014-04-09T20:49:03Z

👍

danizord · 2014-04-09T20:53:50Z

src/ZfcUser/Entity/UserInterface.php

@@ -4,6 +4,7 @@

 interface UserInterface
 {
+


Remove this line.

ClemensSahs · 2014-04-10T07:42:50Z

👎
I disagree, too. The reason for Interfaces is too defined a Methode that must defined public. Which type-hinting is the right is a other question. In the case of UserEntity we need only nativ values.
Yes a value valid by Type-Hinting, is a nice thing but for the actual version is not possible. This will a BC-Break.

But perhaps this is a alternativ for you. Defined a separate method like setEmailObject(EmailAddress $email) and give a casted value to setEmail($email)

texdc · 2014-04-11T07:07:35Z

It seems that you have not bothered to test this change and are operating from a false pretense. Removing a formal declaration from an interface does not prevent you from breaking encapsulation. You're free to continue the mistake of global mutability without the interface imposing it upon you, therefore preserving BC.

On Apr 10, 2014, at 2:42 AM, ClemensSahs notifications@github.com wrote:

I disagree, too. The reason for Interfaces is too defined a Methode that must defined public. Which type-hinting is the right is a other question. In the case of UserEntity we need only nativ values.
Yes a value valid by Type-Hinting, is a nice thing but for the actual version is not possible. This will a BC-Break.

But perhaps this is a alternativ for you. Defined a separate method like setEmailObject(EmailAddress $email) and give a casted value to setEmail($email)

—
Reply to this email directly or view it on GitHub.

Xerkus · 2014-04-11T10:04:18Z

👎
Getters and setters are bad, they expose internal state and blablabla. We all know that, lets remove them.
Well, no.
Look at those entities and their interfaces. They do not follow DDD. They are anemic value containers and setters ARE part of their contract. And that is what those using them expect to see. And that is what they must remain.

PS: If you want to enforce immutability - throw exception in setter if value has changed.

texdc · 2014-04-11T15:41:38Z

Yep, let's just avoid all the clutter and just implement __get() and __set(). Poor design/implementation/practice FTW!

Ocramius · 2014-04-11T18:17:54Z

The setters are indeed not required by the interface, but this is a major BC break (to be marked for 1.x)

How the object is populated is entirely up to the mapper and eventually to the hydrator that is used internally in ZfcUser (through the form), but AFAIK, no other API internal to ZfcUser requires the setters. I fully agree with @texdc here.

If somebody wants to interact with the identity to "write" to it then he should be aware of the specific implementation of the identity itself, and not rely on an interface that was not designed for it and isn't able to handle custom use-cases either (additional fields).

Assuming that the identity is anemic is indeed a mistake of ZfcUser, since it tries to provide a generic implementation, and enforcing an anemic identity is not generic enough.

Ocramius · 2014-04-11T18:37:05Z

@Danielss89 just listed me some methods that are indeed required within ZfcUser... this is indeed annoying :-(

Danielss89 · 2014-04-11T18:47:45Z

@texdc After discussing with a few people i've come to the following conclusion:
You are right that the setters shouldn't be in the interface.
BUT: At this time we are actually doing "setEmail" and "setXXX" a few places, so those are required in the interface.
With that said: If you can refactor the setters away(and make it use the hydrator instead), then i will merge this PR :)

texdc · 2014-04-13T01:09:10Z

@Danielss89 OK, but finish #379 first.

Danielss89 · 2014-04-17T11:26:06Z

@texdc #379 merged.

texdc · 2014-04-17T22:39:48Z

Thanks! I'll start ASAP.

On Apr 17, 2014, at 6:26 AM, Daniel Strøm notifications@github.com wrote:

@texdc #379 merged.

—
Reply to this email directly or view it on GitHub.

Setters are potentially unnecessary and may vary per implementation due to PHP's limitations on type-casting and method signatures.

texdc · 2014-04-27T21:56:27Z

@Danielss89 @Ocramius This should be ready for review/merge. It looks like the build failed because the mocks were not generated properly.

Danielss89 · 2014-04-28T18:56:00Z

Please fix tests, and i will review afterwords.

Danielss89 · 2014-04-28T18:58:55Z

It seems to be a 5.3.3 issue only.

texdc · 2014-04-28T19:14:12Z

I checked the tests. They appear fine. As I said, the mocks were not generated properly by phpunit - their definitions are accurate. If 5.3 passes, 5.3.3 should as well.

On Apr 28, 2014, at 1:59 PM, Daniel Strøm notifications@github.com wrote:

It seems to be a 5.3.3 issue only.

—
Reply to this email directly or view it on GitHub.

Danielss89 · 2014-04-28T19:16:35Z

I just reran them on travis, they still seem to fail

texdc · 2014-05-02T08:21:56Z

I have no control over the failures - they are caused by composer's unreliable loading. No changes have been made to the composer.json and doing a clean install --dev --prefer-source pulls ZF 2.3.1 which includes the AuthenticationServiceInterface. There's no reason for it to be missing on Travis. Further, I do not have access to php 5.3.3 locally without a custom build. The earliest from homebrew is 5.3.28.

$ composer install --dev --prefer-source
Loading composer repositories with package information
Installing dependencies (including require-dev)
  - Installing zendframework/zendxml (1.0.0)
    Cloning 559b34f426d33a11c3db118e00ce14bb8dc64e5f

  - Installing zendframework/zendframework (2.3.1)
    Cloning 055eac4314425c291f3ef0e104a6196a8fc24cd5

  - Installing zendframework/zend-stdlib (2.3.0)
    Cloning d1c481b8a43f2f079b16d3567960ba539e9dacaa

  - Installing zendframework/zend-eventmanager (2.3.0)
    Cloning 048164ee81854942667ffdda3030ce06caa79bb5

  - Installing zendframework/zend-db (2.3.0)
    Cloning 37d573ea83b9573e2247c36fac7b370283ca673e

  - Installing zf-commons/zfc-base (v0.1.2)
    Cloning fc68a49b58ba68584a9a4c44a1ca9df12b30d1bf

  - Installing pdepend/pdepend (1.1.3)
    Cloning 1537f19d62d7b30c13ac173270106df7c6b9c459

  - Installing phpmd/phpmd (1.4.1)
    Cloning 7d03a50e937811970751697ad1aea33a1089d66f

  - Installing symfony/yaml (v2.4.4)
    Cloning 65539ecde838f9c0d18b006b2101e3deb4b5c9ff

  - Installing phpunit/php-text-template (1.2.0)
    Cloning 206dfefc0ffe9cebf65c413e3d0e809c82fbf00a

  - Installing phpunit/phpunit-mock-objects (1.2.3)
    Cloning 5794e3c5c5ba0fb037b11d8151add2a07fa82875

  - Installing phpunit/php-timer (1.0.5)
    Cloning 19689d4354b295ee3d8c54b4f42c3efb69cbc17c

  - Installing phpunit/php-token-stream (1.2.2)
    Cloning ad4e1e23ae01b483c16f600ff1bebec184588e32

  - Installing phpunit/php-file-iterator (1.3.4)
    Cloning acd690379117b042d1c8af1fafd61bde001bf6bb

  - Installing phpunit/php-code-coverage (1.2.17)
    Cloning 6ef2bf3a1c47eca07ea95f0d8a902a6340390b34

  - Installing phpunit/phpunit (3.7.37)
    Cloning ae6cefd7cc84586a5ef27e04bae11ee940ec63dc

  - Installing squizlabs/php_codesniffer (1.4.8)
    Cloning d26daa8096ad2c8758677f0352597f8cda4722e0

zendframework/zendframework suggests installing doctrine/annotations (Doctrine Annotations >=1.0 for annotation features)
zendframework/zendframework suggests installing ircmaxell/random-lib (Fallback random byte generator for Zend\Math\Rand if OpenSSL/Mcrypt extensions are unavailable)
zendframework/zendframework suggests installing ocramius/proxy-manager (ProxyManager 0.5.* to handle lazy initialization of services)
zendframework/zendframework suggests installing zendframework/zendpdf (ZendPdf for creating PDF representations of barcodes)
zendframework/zendframework suggests installing zendframework/zendservice-recaptcha (ZendService\ReCaptcha for rendering ReCaptchas in Zend\Captcha and/or Zend\Form)
phpunit/phpunit suggests installing phpunit/php-invoker (~1.1)
Writing lock file
Generating autoload files

$ ls -l vendor/zendframework/
total 0
drwxr-xr-x   3 george  wheel  102 May  2 02:56 zend-db
drwxr-xr-x   3 george  wheel  102 May  2 02:56 zend-eventmanager
drwxr-xr-x   3 george  wheel  102 May  2 02:56 zend-stdlib
drwxr-xr-x  21 george  wheel  714 May  2 02:56 zendframework
drwxr-xr-x  10 george  wheel  340 May  2 02:54 zendxml

$ ls -l vendor/zendframework/zendframework/library/Zend/Authentication/
total 40
drwxr-xr-x  12 george  wheel   408 May  2 02:56 Adapter
-rw-r--r--   1 george  wheel  4016 May  2 02:56 AuthenticationService.php
-rw-r--r--   1 george  wheel  1007 May  2 02:56 AuthenticationServiceInterface.php
drwxr-xr-x   6 george  wheel   204 May  2 02:56 Exception
-rw-r--r--   1 george  wheel   459 May  2 02:56 README.md
-rw-r--r--   1 george  wheel  2703 May  2 02:56 Result.php
drwxr-xr-x   6 george  wheel   204 May  2 02:56 Storage
drwxr-xr-x   3 george  wheel   102 May  2 02:56 Validator
-rw-r--r--   1 george  wheel  1490 May  2 02:56 composer.json

$ ../vendor/bin/phpunit
PHPUnit 3.7.37 by Sebastian Bergmann.

Configuration read from /Library/WebServer/Documents/ZfcUser/tests/phpunit.xml

...............................................................  63 / 278 ( 22%)
............................................................... 126 / 278 ( 45%)
............................................................... 189 / 278 ( 67%)
............................................................... 252 / 278 ( 90%)
..........................

Time: 8.64 seconds, Memory: 52.50Mb

OK (278 tests, 1379 assertions)

Generating code coverage report in Clover XML format ... done


Code Coverage Report 
  2014-05-02 02:17:45

 Summary: 
  Classes: 85.71% (30/35)
  Methods: 97.47% (193/198)
  Lines:   96.89% (1061/1095)

\ZfcUser\Authentication\Adapter::AbstractAdapter
  Methods: 100.00% ( 4/ 4)   Lines: 100.00% ( 13/ 13)
\ZfcUser\Authentication\Adapter::AdapterChain
  Methods: 100.00% ( 7/ 7)   Lines: 100.00% ( 53/ 53)
\ZfcUser\Authentication\Adapter::AdapterChainEvent
  Methods: 100.00% ( 8/ 8)   Lines: 100.00% ( 17/ 17)
\ZfcUser\Authentication\Adapter::AdapterChainServiceFactory
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 21/ 21)
\ZfcUser\Authentication\Adapter::Db
  Methods: 100.00% (14/14)   Lines: 100.00% ( 91/ 91)
\ZfcUser\Authentication\Storage::Db
  Methods: 100.00% (10/10)   Lines:  87.18% ( 34/ 39)
\ZfcUser\Controller::UserController
  Methods: 100.00% (19/19)   Lines:  98.95% (188/190)
\ZfcUser\Controller\Plugin::ZfcUserAuthentication
  Methods: 100.00% ( 6/ 6)   Lines: 100.00% (  8/  8)
\ZfcUser\Entity::User
  Methods: 100.00% (12/12)   Lines: 100.00% ( 18/ 18)
\ZfcUser\Form::Base
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% ( 54/ 54)
\ZfcUser\Form::ChangeEmail
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 35/ 35)
\ZfcUser\Form::ChangeEmailFilter
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 30/ 30)
\ZfcUser\Form::ChangePassword
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 39/ 39)
\ZfcUser\Form::ChangePasswordFilter
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% ( 47/ 47)
\ZfcUser\Form::Login
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 34/ 34)
\ZfcUser\Form::LoginFilter
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% ( 23/ 23)
\ZfcUser\Form::Register
  Methods: 100.00% ( 4/ 4)   Lines:  90.00% ( 18/ 20)
\ZfcUser\Form::RegisterFilter
  Methods: 100.00% ( 7/ 7)   Lines: 100.00% ( 71/ 71)
\ZfcUser\Mapper::User
  Methods: 100.00% ( 7/ 7)   Lines: 100.00% ( 26/ 26)
\ZfcUser\Mapper::UserHydrator
  Methods: 100.00% ( 6/ 6)   Lines:  90.91% ( 20/ 22)
\ZfcUser\Options::ModuleOptions
  Methods: 100.00% (40/40)   Lines: 100.00% ( 60/ 60)
\ZfcUser\Service::User
  Methods:  94.44% (17/18)   Lines:  92.68% ( 76/ 82)
\ZfcUser\Validator::AbstractRecord
  Methods: 100.00% ( 6/ 6)   Lines: 100.00% ( 23/ 23)
\ZfcUser\Validator::NoRecordExists
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% (  8/  8)
\ZfcUser\Validator::RecordExists
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% (  8/  8)
\ZfcUser\View\Helper::ZfcUserDisplayName
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% ( 22/ 22)
\ZfcUser\View\Helper::ZfcUserIdentity
  Methods: 100.00% ( 3/ 3)   Lines: 100.00% (  6/  6)
\ZfcUser\View\Helper::ZfcUserLoginWidget
  Methods: 100.00% ( 4/ 4)   Lines: 100.00% ( 21/ 21)

$ php -v
PHP 5.3.28 (cli) (built: May  2 2014 00:54:57) (DEBUG)
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2013 Zend Technologies
    with Xdebug v2.2.5, Copyright (c) 2002-2014, by Derick Rethans

Clone the repo/PR and run the tests under 5.3.* locally. If you can make them fail like Travis, let me know.

Ocramius · 2014-05-05T00:05:14Z

@Danielss89 do we really need 5.3.3 after this refactoring? We could just bump like ZF2 did...

Danielss89 · 2014-05-06T14:54:11Z

@Ocramius good point. Let's just remove it, @texdc can you do that, then i think its ready for merge.

danizord reviewed Apr 9, 2014
View reviewed changes

src/ZfcUser/Entity/UserInterface.php

@@ -4,6 +4,7 @@

interface UserInterface

{

Copy link

Member

danizord Apr 9, 2014

Choose a reason for hiding this comment

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

Remove this line.

Danielss89 added BC break labels Apr 11, 2014

Danielss89 added this to the 0.2.0 milestone Apr 11, 2014

George Cooksey and others added 10 commits April 26, 2014 21:43

Remove setters from UserInterface

9d5ee26

Setters are potentially unnecessary and may vary per implementation due to PHP's limitations on type-casting and method signatures.

Remove extraneous empty lines

edda30f

Ignore eclipse project files

5b28b88

Update UserHydrator

d86e26b

Typecast UserEntity properties

631dc05

Add HydratorInterface

bd79eac

Refactor User Service to use Hydrator

7f29a51

Clean abstract authentication adapter

4259f7e

Refactor Db adapter for hydrator

24e0bcf

CS and v5.3 fixes

dcdedcf

Fix (?) mock errors

cf281f9

Remove 5.3.3 build

8dbe79b

Danielss89 merged commit 8dbe79b into ZF-Commons:master May 11, 2014

ojhaujjwal mentioned this pull request Nov 3, 2014

Interface doesn't have all the required methods: "\ZfcUser\Entity\UserInterface" #540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove setters from UserInterface #367

Remove setters from UserInterface #367

texdc commented Nov 5, 2013

JanMalte commented Nov 20, 2013

texdc commented Nov 20, 2013

danizord commented Apr 9, 2014

danizord Apr 9, 2014

ClemensSahs commented Apr 10, 2014

texdc commented Apr 11, 2014

Xerkus commented Apr 11, 2014

texdc commented Apr 11, 2014

Ocramius commented Apr 11, 2014

Ocramius commented Apr 11, 2014

Danielss89 commented Apr 11, 2014

texdc commented Apr 13, 2014

Danielss89 commented Apr 17, 2014

texdc commented Apr 17, 2014

texdc commented Apr 27, 2014

Danielss89 commented Apr 28, 2014

Danielss89 commented Apr 28, 2014

texdc commented Apr 28, 2014

Danielss89 commented Apr 28, 2014

texdc commented May 2, 2014

Ocramius commented May 5, 2014

Danielss89 commented May 6, 2014

Remove setters from UserInterface #367

Remove setters from UserInterface #367

Conversation

texdc commented Nov 5, 2013

JanMalte commented Nov 20, 2013

texdc commented Nov 20, 2013

danizord commented Apr 9, 2014

danizord Apr 9, 2014

Choose a reason for hiding this comment

ClemensSahs commented Apr 10, 2014

texdc commented Apr 11, 2014

Xerkus commented Apr 11, 2014

texdc commented Apr 11, 2014

Ocramius commented Apr 11, 2014

Ocramius commented Apr 11, 2014

Danielss89 commented Apr 11, 2014

texdc commented Apr 13, 2014

Danielss89 commented Apr 17, 2014

texdc commented Apr 17, 2014

texdc commented Apr 27, 2014

Danielss89 commented Apr 28, 2014

Danielss89 commented Apr 28, 2014

texdc commented Apr 28, 2014

Danielss89 commented Apr 28, 2014

texdc commented May 2, 2014

Ocramius commented May 5, 2014

Danielss89 commented May 6, 2014