Add a bunch of traits to ZF2 #2927

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

Mezzle commented Nov 9, 2012

As requested @

protecinnovations/zf2-traits#1

This is the initial commit to start getting traits into ZF2.

We need to move our tests over - which are currently using Mockery.

@EvanDotPro EvanDotPro and 2 others commented on an outdated diff Nov 9, 2012

library/Zend/ServiceManager/ServiceManagerAwareTrait.php
+ */
+
+namespace Zend\ServiceManager;
+
+use Zend\ServiceManager\ServiceManager;
+
+/**
+ * @category Zend
+ * @package Zend_ServiceManager
+ */
+trait ServiceManagerAwareTrait
+{
+ /**
+ * @var \Zend\ServiceManager\ServiceManager
+ */
+ protected $service_manager = null;
@EvanDotPro

EvanDotPro Nov 9, 2012

Member

Camel case please. Also, the var docblock can just be ServiceManager since there's a use statement.

@Mezzle

Mezzle Nov 9, 2012

Contributor

Personally, I prefer having the full path in the docblock - then I don't have to go to the top of a file to find which class I'm actually using, lose my place where I was editing... etc etc...

But as I said in IRC:

* Mez awaits the inevitable "but our coding standards don't match!"
< EvanDotPro> Mez: Mez lol
< EvanDotPro> just left a comment :-p
@weierophinney

weierophinney Nov 10, 2012

Owner

Please, no ServiceManagerAware traits - do SeviceLocatorAware instead, as
that's what we standardized on.
On Nov 9, 2012 9:19 AM, "Evan Coury" notifications@github.com wrote:

In library/Zend/ServiceManager/ServiceManagerAwareTrait.php:

  • /
    +
    +namespace Zend\ServiceManager;
    +
    +use Zend\ServiceManager\ServiceManager;
    +
    +/
    *
  • * @category Zend
  • * @Package Zend_ServiceManager
  • */
    +trait ServiceManagerAwareTrait
    +{
  • /**
  • \* @var \Zend\ServiceManager\ServiceManager
    
  • */
    
  • protected $service_manager = null;

Camel case please. Also, the var docblock can just be ServiceManager since
there's a use statement.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/2927/files#r2084108.

@bakura10 bakura10 commented on the diff Nov 9, 2012

library/Zend/EventManager/EventManagerAwareTrait.php
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_EventManager
+ */
+
+namespace Zend\EventManager;
+
+use Zend\EventManager\EventManagerInterface;
+
+/**
+ * @category Zend
+ * @package Zend_EventManager
+ */
+trait EventManagerAwareTrait
@bakura10

bakura10 Nov 9, 2012

Contributor

This one does not already exists ? The ProvidesEvent trait ?

@Mezzle

Mezzle Nov 9, 2012

Contributor

Sort of...

I've just done a basic copy over of our stuff - the main reason we didn't use ProvidesEvent traits is due to the fact that ProvidesEvent is not neccesarily correct. It may just need to know about the Event Manager (for example, for adding listeners). So therefore, EventManagerAwareTrait is more technically correct (although the code should probably be bought in line with ProvidesEvent)

@bakura10

bakura10 Nov 9, 2012

Contributor

Ok. Anyway, we should agree on a naming too for traits. I like the "Provides" something naming.

@Mezzle

Mezzle Nov 9, 2012

Contributor

Our thinking was that it's easy just to s/Interface/Trait/ and that it's not really "Providing" anything - the trait just makes implements the Awareness....

So these traits make things that are aware of things actually aware of them...

s/Trait/Skynet/

@weierophinney

weierophinney Nov 19, 2012

Owner

@Mezzle The point is that ProvidesEvents already exists, and does exactly what this one does. One should use the other, and that way they can be synonyms/substitutions for each other.

Contributor

Slamdunk commented Nov 9, 2012

Please can you provide tests, and test skips when testing on php < 5.4 ?

Contributor

Mezzle commented Nov 9, 2012

Slamdunk - please see initial comment - we still need to provide tests (hence WIP) :) Our old tests ultilise Mockery - so @alexdenvir is currently rewriting them :)

Mezzle referenced this pull request in protecinnovations/zf2-traits Nov 9, 2012

Closed

Contribute to ZF2 #1

Contributor

Slamdunk commented Nov 10, 2012

You can use @requires before test class declaration, so you won't need it in every test :)

Contributor

Mezzle commented Nov 10, 2012

Slamdunk - unfortunately - we tried that and it didn't work (it tried to load the object)

Contributor

Slamdunk commented Nov 10, 2012

I see you are using a lot of assets: why don't you use getObjectForTrait (http://sebastian-bergmann.de/archives/906-Testing-Traits.html) ?

Mezzle closed this Nov 14, 2012

Mezzle reopened this Nov 14, 2012

Martin Meredith and others added some commits Nov 9, 2012

Martin Meredith Add a bunch of traits to ZF2
As requested @

protecinnovations/zf2-traits#1

This is the initial commit to start getting traits into ZF2.

We need to move our tests over - which are currently using Mockery.
331151c
@alexdenvir alexdenvir Improve coding standards on new traits, add tests c568239
Martin Meredith Fix Minor problems with tests b0e0b89

@Ocramius Ocramius commented on an outdated diff Nov 19, 2012

library/Zend/I18n/Translator/TranslatorAwareTrait.php
+ * @param string $textDomain
+ * @return mixed
+ */
+ public function setTranslator(Translator $translator = null, $textDomain = null)
+ {
+ $this->translator = $translator;
+
+ if (!is_null($textDomain)) {
+ $this->setTranslatorTextDomain($textDomain);
+ }
+
+ return $this;
+ }
+
+ /**
+ * getTranslator
@Ocramius

Ocramius Nov 19, 2012

Member

Those comments are quite useless... Could you fix them with real text (or strip them?)

@weierophinney weierophinney commented on an outdated diff Nov 19, 2012

...ry/Zend/EventManager/SharedEventManagerAwareTrait.php
@@ -0,0 +1,60 @@
@weierophinney

weierophinney Nov 19, 2012

Owner

I'm not 100% convinced we need this one; the only place you'd really need it is if you were creating a custom EventManagerInterface implementation.

@weierophinney weierophinney commented on an outdated diff Nov 19, 2012

library/Zend/ServiceManager/ServiceManagerAwareTrait.php
@@ -0,0 +1,38 @@
@weierophinney

weierophinney Nov 19, 2012

Owner

Definitely should not have this one; we're promoting ServiceLocatorAwareInterface as the only interface you should utilize inside your code.

Owner

weierophinney commented Nov 19, 2012

This is looking great -- with the few changes suggested, I'll be ready to merge.

Contributor

alexdenvir commented Nov 20, 2012

@weierophinney I've removed the two traits you were unsure of. Did you also want me to make changes to EventManagerAwareTrait/ProvidesEvents like suggested in the above discussion as well?

Owner

weierophinney commented Nov 20, 2012

Yes, please. You can simply have one use the other.

On Tuesday, November 20, 2012, Alex Denvir wrote:

@weierophinney https://github.com/weierophinney I've removed the two
traits you were unsure of. Did you also want me to make changes to
EventManagerAwareTrait/ProvidesEvents like suggested in the above
discussion as well?


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/2927#issuecomment-10550922.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

Contributor

alexdenvir commented Nov 20, 2012

@weierophinney Done :) EventManagerAwareTrait now uses ProvidesEvents

Owner

weierophinney commented Nov 20, 2012

Merged to develop branch, for 2.1.0.

@weierophinney weierophinney added a commit to zendframework/zend-eventmanager that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/2927' into develop 9af5add

@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/2927' into develop fdab45c

@weierophinney weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/2927' into develop a0d7134

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/2927' into develop 5ef112f

@weierophinney weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/2927' into develop 5576455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment