Zend\Loader\PluginBroker

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Zend\Loader\PluginBroker

Marc Bennewitz (private)
Hi all,

I'm currently working on changing the default behavior of
Zend\Loader\PluginBroker::$registerPluginsOnLoad to false.

That means after changing it all used plugin broker not setting this
options no longer holds already loaded instances. Because of it's often
used on static factories this option should be false but I don't see it
has been done.

I would like to inform all component maintainers to review this option,
setting it explicit and send a PR to
https://github.com/marc-mabe/zf2/tree/pluginBroker

Components:
Zend\Acl (I set it already to true)
Zend\Barcode
Zend\Cache (I set it already to false)
Zend\Filter
Zend\Mail
Zend\Markup
Zend\Mvc
Zend\Paginator
Zend\Serializer (I set it already to false)
Zend\Tag
Zend\View

Greetings
Marc

Reply | Threaded
Open this post in threaded view
|

Re: Zend\Loader\PluginBroker

weierophinney
Administrator
-- Marc Bennewitz <[hidden email]> wrote
(on Sunday, 11 March 2012, 09:39 PM +0100):
> I'm currently working on changing the default behavior of
> Zend\Loader\PluginBroker::$registerPluginsOnLoad to false.

Why?

The original reason for having this enabled by default is that in most
cases, we want to re-use instances. I know this is true of the MVC and
View layers in particular, and, IIRC, expected behavior in Mai,
Paginator, and Tag.

I would argue instead that you set the flag to false in the specific
implementation for a component:

    namespace Zend\Cache;

    class HelperBroker
    {
        public $registerPluginsOnLoad = false;
    }

This was the intended way to override the behavior in specific
components.

> That means after changing it all used plugin broker not setting this
> options no longer holds already loaded instances. Because of it's often
> used on static factories this option should be false but I don't see it
> has been done.
>
> I would like to inform all component maintainers to review this option,
> setting it explicit and send a PR to
> https://github.com/marc-mabe/zf2/tree/pluginBroker
>
> Components:
> Zend\Acl (I set it already to true)
> Zend\Barcode
> Zend\Cache (I set it already to false)
> Zend\Filter
> Zend\Mail
> Zend\Markup
> Zend\Mvc
> Zend\Paginator
> Zend\Serializer (I set it already to false)
> Zend\Tag
> Zend\View
>
> Greetings
> Marc
>

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Reply | Threaded
Open this post in threaded view
|

Re: Zend\Loader\PluginBroker

Marc Bennewitz (private)
In reply to this post by Marc Bennewitz (private)
Matthew,

for components reusing loaded instances I'm with you but on components using it from a factory
it's not.

Sure there is no problem if the component explicit set it to false but for now only Zend\Paginator & Zend\Cache (since today) do it. - I'm sure more components should.

The only problem is that this behavior is vary hard to discover because factories are often static.
I takes hours finding out why tests fails on running the hole test suite and on testing only the failed component all was fine.

We also talked about it for over a week ;)
[2012-03-01 21:29:36] <mabe_> weierophinney: About "register_plugins_on_load" of PluginBroker: It's set to true by default and wasn't set well within the static cache factory I and PadraicB need hours to find out why the complete test suite fails with errors which wasn't raised on running single components ... can we simple set it false by default ?
<snip>
[2012-03-01 21:34:51] <weierophinney> mabe_, yes, that seems reasonable. Though we'll need to ensure places where it's expected to be on override the setting.
[2012-03-01 21:35:49] <mabe_> weierophinney: I'll search for it and create a PR - thanks


Greetings
Marc

On 12.03.2012 17:07, weierophinney [via Zend Framework Community] wrote:
-- Marc Bennewitz <[hidden email]> wrote
(on Sunday, 11 March 2012, 09:39 PM +0100):
> I'm currently working on changing the default behavior of
> Zend\Loader\PluginBroker::$registerPluginsOnLoad to false.

Why?

The original reason for having this enabled by default is that in most
cases, we want to re-use instances. I know this is true of the MVC and
View layers in particular, and, IIRC, expected behavior in Mai,
Paginator, and Tag.

I would argue instead that you set the flag to false in the specific
implementation for a component:

    namespace Zend\Cache;

    class HelperBroker
    {
        public $registerPluginsOnLoad = false;
    }

This was the intended way to override the behavior in specific
components.

> That means after changing it all used plugin broker not setting this
> options no longer holds already loaded instances. Because of it's often
> used on static factories this option should be false but I don't see it
> has been done.
>
> I would like to inform all component maintainers to review this option,
> setting it explicit and send a PR to
> https://github.com/marc-mabe/zf2/tree/pluginBroker
>
> Components:
> Zend\Acl (I set it already to true)
> Zend\Barcode
> Zend\Cache (I set it already to false)
> Zend\Filter
> Zend\Mail
> Zend\Markup
> Zend\Mvc
> Zend\Paginator
> Zend\Serializer (I set it already to false)
> Zend\Tag
> Zend\View
>
> Greetings
> Marc
>

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc



If you reply to this email, your message will be added to the discussion below:
http://zend-framework-community.634137.n4.nabble.com/Zend-Loader-PluginBroker-tp4464561p4466474.html
To start a new topic under ZF Contributor, email [hidden email]
To unsubscribe from ZF Contributor, click here.
NAML
Reply | Threaded
Open this post in threaded view
|

Re: Zend\Loader\PluginBroker

weierophinney
Administrator
-- Marc Bennewitz <[hidden email]> wrote
(on Monday, 12 March 2012, 09:46 PM +0100):
> for components reusing loaded instances I'm with you but on components
> using it from a factory it's not.

Why? The broker is intended to act as:

 * Locator
 * Factory
 * Registry

The last is an explicit design goal of it, and it's why we have that
flag -- because of the recognition that in some cases, shared instances
are not preferred. However, again, the reason it's default behavior is
to share instances is because the bulk of locations in the framework
that a broker is required should use shared instances. For those
components that do not want them, we default it off in that component's
broker.

> Sure there is no problem if the component explicit set it to false but
> for now only Zend\Paginator & Zend\Cache (since today) do it. - I'm
> sure more components should.

Which ones?

> The only problem is that this behavior is vary hard to discover
> because factories are often static.  

The brokers are not static factories, though. They are per-instance. If
you're having issues with static state, something else entirely is
happening.

> I takes hours finding out why tests fails on running the hole test
> suite and on testing only the failed component all was fine.

Can you give some specific examples?

> We also talked about it for over a week ;)
> [2012-03-01 21:29:36] <mabe_> weierophinney: About "register_plugins_on_load"
> of PluginBroker: It's set to true by default and wasn't set well within the
> static cache factory I and PadraicB need hours to find out why the complete
> test suite fails with errors which wasn't raised on running single components
> ... can we simple set it false by default ?
> <snip>
> [2012-03-01 21:34:51] <weierophinney> mabe_, yes, that seems reasonable. Though
> we'll need to ensure places where it's expected to be on override the setting.
> [2012-03-01 21:35:49] <mabe_> weierophinney: I'll search for it and create a PR
> - thanks

I was thinking this was about a different setting entirely -- I'm sorry
if I misled you.

> On 12.03.2012 17:07, weierophinney [via Zend Framework Community] wrote:
>
>     -- Marc Bennewitz <[hidden email]> wrote
>     (on Sunday, 11 March 2012, 09:39 PM +0100):
>     > I'm currently working on changing the default behavior of
>     > Zend\Loader\PluginBroker::$registerPluginsOnLoad to false.
>
>     Why?
>
>     The original reason for having this enabled by default is that in most
>     cases, we want to re-use instances. I know this is true of the MVC and
>     View layers in particular, and, IIRC, expected behavior in Mai,
>     Paginator, and Tag.
>
>     I would argue instead that you set the flag to false in the specific
>     implementation for a component:
>
>         namespace Zend\Cache;
>
>         class HelperBroker
>         {
>             public $registerPluginsOnLoad = false;
>         }
>
>     This was the intended way to override the behavior in specific
>     components.
>
>     > That means after changing it all used plugin broker not setting this
>     > options no longer holds already loaded instances. Because of it's often
>     > used on static factories this option should be false but I don't see it
>     > has been done.
>     >
>     > I would like to inform all component maintainers to review this option,
>     > setting it explicit and send a PR to
>     > https://github.com/marc-mabe/zf2/tree/pluginBroker
>     >
>     > Components:
>     > Zend\Acl (I set it already to true)
>     > Zend\Barcode
>     > Zend\Cache (I set it already to false)
>     > Zend\Filter
>     > Zend\Mail
>     > Zend\Markup
>     > Zend\Mvc
>     > Zend\Paginator
>     > Zend\Serializer (I set it already to false)
>     > Zend\Tag
>     > Zend\View
>     >
>     > Greetings
>     > Marc
>     >
>
>     --
>     Matthew Weier O'Phinney
>     Project Lead            | [hidden email]
>     Zend Framework          | http://framework.zend.com/
>     PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
>
>
>     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
>     If you reply to this email, your message will be added to the discussion
>     below:
>     http://zend-framework-community.634137.n4.nabble.com/
>     Zend-Loader-PluginBroker-tp4464561p4466474.html
>     To start a new topic under ZF Contributor, email
>     [hidden email]
>     To unsubscribe from ZF Contributor, click here.
>     NAML
>

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Reply | Threaded
Open this post in threaded view
|

Re: Zend\Loader\PluginBroker

Marc Bennewitz (private)
On 12.03.2012 22:37, Matthew Weier O'Phinney wrote:

> -- Marc Bennewitz <[hidden email]> wrote
> (on Monday, 12 March 2012, 09:46 PM +0100):
>> for components reusing loaded instances I'm with you but on components
>> using it from a factory it's not.
> Why? The broker is intended to act as:
>
>  * Locator
>  * Factory
>  * Registry
>
> The last is an explicit design goal of it, and it's why we have that
> flag -- because of the recognition that in some cases, shared instances
> are not preferred. However, again, the reason it's default behavior is
> to share instances is because the bulk of locations in the framework
> that a broker is required should use shared instances. For those
> components that do not want them, we default it off in that component's
> broker.
>
>> Sure there is no problem if the component explicit set it to false but
>> for now only Zend\Paginator & Zend\Cache (since today) do it. - I'm
>> sure more components should.
> Which ones?
I'm not sure 100% but:
Zend\Barcode\Barcore::factory
Zend\Markup\Markup::factory
Zend\Serializer\Serializer::factory

>
>> The only problem is that this behavior is vary hard to discover
>> because factories are often static.  
> The brokers are not static factories, though. They are per-instance. If
> you're having issues with static state, something else entirely is
> happening.
Sure it have to set to false on using it on static factories.

>
>> I takes hours finding out why tests fails on running the hole test
>> suite and on testing only the failed component all was fine.
> Can you give some specific examples?
before today:
$cache1 = Zend\Cache\StorageFactory::adapterFactory('filesystem',
array('cache_dir' => '/tmp'));
$cache2 = Zend\Cache\StorageFactory::adapterFactory('filesystem',
array('cache_dir' => '/other'));
var_dump($cache1 === $cache2);

>
>> We also talked about it for over a week ;)
>> [2012-03-01 21:29:36] <mabe_> weierophinney: About "register_plugins_on_load"
>> of PluginBroker: It's set to true by default and wasn't set well within the
>> static cache factory I and PadraicB need hours to find out why the complete
>> test suite fails with errors which wasn't raised on running single components
>> ... can we simple set it false by default ?
>> <snip>
>> [2012-03-01 21:34:51] <weierophinney> mabe_, yes, that seems reasonable. Though
>> we'll need to ensure places where it's expected to be on override the setting.
>> [2012-03-01 21:35:49] <mabe_> weierophinney: I'll search for it and create a PR
>> - thanks
> I was thinking this was about a different setting entirely -- I'm sorry
> if I misled you.
>
>> On 12.03.2012 17:07, weierophinney [via Zend Framework Community] wrote:
>>
>>     -- Marc Bennewitz <[hidden email]> wrote
>>     (on Sunday, 11 March 2012, 09:39 PM +0100):
>>     > I'm currently working on changing the default behavior of
>>     > Zend\Loader\PluginBroker::$registerPluginsOnLoad to false.
>>
>>     Why?
>>
>>     The original reason for having this enabled by default is that in most
>>     cases, we want to re-use instances. I know this is true of the MVC and
>>     View layers in particular, and, IIRC, expected behavior in Mai,
>>     Paginator, and Tag.
>>
>>     I would argue instead that you set the flag to false in the specific
>>     implementation for a component:
>>
>>         namespace Zend\Cache;
>>
>>         class HelperBroker
>>         {
>>             public $registerPluginsOnLoad = false;
>>         }
>>
>>     This was the intended way to override the behavior in specific
>>     components.
>>
>>     > That means after changing it all used plugin broker not setting this
>>     > options no longer holds already loaded instances. Because of it's often
>>     > used on static factories this option should be false but I don't see it
>>     > has been done.
>>     >
>>     > I would like to inform all component maintainers to review this option,
>>     > setting it explicit and send a PR to
>>     > https://github.com/marc-mabe/zf2/tree/pluginBroker
>>     >
>>     > Components:
>>     > Zend\Acl (I set it already to true)
>>     > Zend\Barcode
>>     > Zend\Cache (I set it already to false)
>>     > Zend\Filter
>>     > Zend\Mail
>>     > Zend\Markup
>>     > Zend\Mvc
>>     > Zend\Paginator
>>     > Zend\Serializer (I set it already to false)
>>     > Zend\Tag
>>     > Zend\View
>>     >
>>     > Greetings
>>     > Marc
>>     >
>>
>>     --
>>     Matthew Weier O'Phinney
>>     Project Lead            | [hidden email]
>>     Zend Framework          | http://framework.zend.com/
>>     PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
>>
>>
>>     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
>>     If you reply to this email, your message will be added to the discussion
>>     below:
>>     http://zend-framework-community.634137.n4.nabble.com/
>>     Zend-Loader-PluginBroker-tp4464561p4466474.html
>>     To start a new topic under ZF Contributor, email
>>     [hidden email]
>>     To unsubscribe from ZF Contributor, click here.
>>     NAML
>>