new Object() vs Object::create() - are we encouraging ::create()?

[Loz Calver]

This is a question that has been in the back of my mind for a little while: are we advocating Object::create() over new Object() wherever possible? Or are are we instead using it somewhat sparingly?

I, personally, much prefer the ::create() syntax: the ability to swap out one class for another is really useful (as I recently found out with the DMS module), plus prettier method chaining is an added bonus!

I vaguely recall seeing a discussion (that I, predictably, can't find) about this before. Is there any (noticeable/significant) performance overhead of using Object::create()? If not, surely we should be making sure that core code is rewritten with this syntax more than we are currently.

As always, I’d be happy to do some work on this (not forgetting updating documentation that mostly uses new Object()) to get it done if that’s the desired way to go.

Thoughts? :)

Loz

[Zauberfisch]

simon_w and I are frequently fighting over this in IRC, perhaps that's
the discussion you refer to.

I am strongly in favour of Object::create() because of the injector
magic that comes with it (as you pointed out already).

What people do in their mysite/ is none of my business. but I would
really like module developers to use ::create() everywhere (or at
least everywhere it makes sense)
and even more in the core. there are a lot of places where it would be
really useful to have it in place.

the only concern I can see is performance. has anyone benchmarked what
big of an performance impact it is to use ::create() all the way?

if the performance impact is tolerate-able, I would vote to make
::create() over new part of our coding guidelines.

> -- You received this message because you are subscribed to the
> Google Groups "SilverStripe Core Development" group. To unsubscribe
> from this group and stop receiving emails from it, send an email to
> silverstripe-d...@googlegroups.com
> <mailto:silverstripe-d...@googlegroups.com>. To post to
> this group, send email to silverst...@googlegroups.com
> <mailto:silverst...@googlegroups.com>. Visit this group at
> http://groups.google.com/group/silverstripe-dev. For more options,
> visit https://groups.google.com/d/optout.

[vikas srivastava]

I will only care about 'performance impact' for syntax vs performance. I had same question in my mind but for performance context only.

Vikas Srivastava

Software Engineer | MoonPeak Media

Email: vi...@moonpeakmedia.com

Skype: viky2130

twitter: @openbeesV

about.me/vikassrivastava

To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.

[Martimiz]

This is by no means a proper benchmark, and I don't know if/where caching kicks in, but, just as a hint, a simple loop creating 10.000 SiteTree objects on my workstation fairly consistently showed about a factor 1.085 time increase:

3.62239599228 s   (new)
3.93376302719 s   (create)

[Nicolaas Thiemen Francken - Sunny Side Up]

I have started to use ::create in my OS modules. 

what are simon_w's arguments?  It would be good to get the full picture.

Nicolaas

[Loz Calver]

I set up a quick test for this (just ReflectionClass, not using Injector or anything) - https://gist.github.com/kinglozzer/bd3d36302d69d8756734. From the results on my dev machine (OS X, PHP 5.5.15) it seems:

• There’s significant overhead the first time an object is created - regardless of whether ReflectionClass is used
• Subsequent ReflectionClass instantiation has much smaller, but still noticeable, overhead with constructor arguments (takes ~200-300% of the time of new Class())
• There’s little-to-no overhead when using ReflectionClass to create an instance of a class with no constructor arguments

It’s worth bearing in mind that there’s a bit more overhead from Injector itself that’s not taken into account here. I’ve also not looked at memory footprint yet, I know that’s also a big concern of the framework. If I get time I might try it with XHProf instead - I’m not entirely convinced of the accuracy of these results.

I think it’d be good to hear from devs who understand this better than I (perhaps someone with knowledge of PHP internals).

[Andrew Short]

I'm not really a big fan of preferring the create syntax - I see it as a way of doing fragile half baked dependency injection. Sure it's often useful, but I think it generally makes code less clear, more difficult to debug, and causes issues when multiple modules will want to monkey patch the same class. My preference is generally to design around interfaces and well defined extension points rather than just replace random classes to make things work. Sure it's more work, but it means the code is more predictable.

Andrew Short.

--

[Sam Minnée]

What I feel like you're saying is "I prefer neither: use Injector::inst()->get("<interface>") or get the Injector config to supply parameters instead!"

I'd agree with that, although I would say that, where you have a base class rather than an interface, <class>::create() is a bit more concise than Injector::inst()->get("<class>"). That said, it's probably better to provide a property setter filled out by Injector and never create your own object.

[Marcus Nyeholt]

A couple points on the injector front

One thing that is very simple to add that I'd like to see the framework make use of would be the automatic binding of an 'injector' property for all objects created by it, so that instead of doing Injector::inst(), you do $this->injector->get()/create(), which would allow for custom dependency injectors to be hooked into place. Given one of the goals of dependency injection is to do away with static references, this seems to me like a better practice :) 

From a functionality standpoint, the injector supports two scopes of object creation - singleton or prototype. (if you're familiar with spring, the 'singleton' type is actually more like the 'request' bean scope, given PHP's stateless nature). Where no specific configuration is defined for items, calling $injector->get('ClassName') will create a new object of type "ClassName", store that in a local map at index 'ClassName' and return the object. Subsequent calls to $injector->get('ClassName') will return that same object. If, however, you define some injector config that looks like

Injector:

  ClassName:

    type: prototype

then each call to $injector->get('ClassName') will always create a new object. Now, if that config wasn't in place, and you always wanted a new object created, you can call $injector->create('ClassName') and it will create a new instance of the object. 

The reason for the existence of the "prototype" option is to allow the full config of dependency wiring to happen in config; while you may be able to write your code in a way that you know you'll always want to call $injector->create(), there are cases where that decision doesn't need to be made at that level and you may actually want a mix between singleton and new instances. 

Injector:

  ClassName:

    type: prototype

    properties: 

      log: %$ProductionLogger

  ProductionLogger: 

    class: SomeLogProvider

    properties:

      email: so...@address.com

      file: /prod/website/path.log

  DevelopmentLogger:

    class: SomeLogProvider

    properties:

      level: everything_including_passwords_wooo

--

[Daniel Hensby]

Ok, so is there actually a decision on this? It seems clear to me that `new Object()` is out of favour, but what about create vs injector?

I think using the create syntax is lovely. Writing Injector::inst()->create('ClassName') is so ugly and ClassName::create() does the same thing (bar this odd strong_classes stuff, which needs to be removed) in a much prettier way.

It's fairly important from a documentation point of view too, because that needs to be consistent and really dictated in the core coding conventions.

[Sam Minnée]

Ok, so is there actually a decision on this?

Not so far ;-)

 

It seems clear to me that `new Object()` is out of favour, but what about create vs injector?

I think using the create syntax is lovely. Writing Injector::inst()->create('ClassName') is so ugly and ClassName::create() does the same thing (bar this odd strong_classes stuff, which needs to be removed) in a much prettier way.

Marcus' point about avoiding static calls where possible is valid, and having $this->injector instead of Injector::inst() would help with that.

However, a concise and enjoyable language for devs to use is also important.

 

It's fairly important from a documentation point of view too, because that needs to be consistent and really dictated in the core coding conventions.

At the end of the day,  I'm not sure that we'll get a one-size-fits-all answer here, but we do need to have an answer to the question "what do we put in documentation, examples, and core code?"

[Michael Strong]

This is slightly related: https://github.com/silverstripe-australia/silverstripe-versionedfiles/pull/42

As you can see, there can be issues with using ::create() everywhere so I don't think its a simple decision of saying 'yes' or no'. Context would always have to be taken into account.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Michael Strong | Platform Support Developer
SilverStripe
http://silverstripe.com/

Phone: +64 4 978 7330
Skype: micmania1

[Daniel Hensby]

At the end of the day,  I'm not sure that we'll get a one-size-fits-all answer here, but we do need to have an answer to the question "what do we put in documentation, examples, and core code?"

True, but that question is the one I'd like an answer to :) 

 

As you can see, there can be issues with using ::create() everywhere so I don't think its a simple decision of saying 'yes' or no'.

Hmm, that's an interesting PR, but I don't think it's the right solution to the problem, it just fixes a symptom of a bigger issue (how modules can play nice together). 

Daniel Hensby Director

e: daniel...@betterbrief.co.uk t:  020 7183 9266 w: http://www.betterbrief.co.uk

[Marcus Nyeholt]

I actually don't mind the ClassType::create() syntax (exceptions to rules and all that :) - its usage context  is quite different than something like Injector::inst or DB::query(), that is, taking advantage of a language feature to make code more 'pleasant' to grok vs  a thin veneer around global function calls. The downside being that something must be a descendent of Object to make use of it. 

[Will Morgan]

If we're debating what should become a standard practice, then ::create should be dropped in favour of something more expressive and transparent. If we've got people using it purely for chaining, then I'd kindly suggest their priorities might need reviewing!

Having some injector auto-property setting would be tons better than ::create. While I don't think performance is really an argument right now, because it doesn't take long to instantiate an object compared to querying the database, having some degree of control over the injector you're using sounds sensible - you don't get that when using ::create.

Overall the injector and the ::create wrapper functionality needs documenting so people know when and why to use it, and any repercussions. For example, using ::create then replacing classes entirely breaks core PHP functionality - like instanceof, and so on.

tl;dr deprecate create and commit to getting Injector up to scratch

[Anselm Christophersen]

Maybe I missed something, but I usually prefer "$obj = new myclass()" because I then get all the code hinting in my IDE. That's the main reason why I'm sticking with this so far.

Sent from my iPhone

--

[Damian Mooyman]

Actually that's why I like Object::create. With an IDE that properly supports PHPDoc you get complete typehinting.

Interoperability when using substituted classes isn't a problem as long as the substitutability principle is maintained (i.e. classes are only replaced with custom subclasses).

The problem I have with Object::create / Injector::inst()->create is that they are both performance hits. On the other hand, I would prefer to optimise the performance for object creation (config API and so on) rather than deprecate the functionality.

Damian Mooyman | Senior Platform Developer

SilverStripe

http://silverstripe.com/

Phone: +64 9 281 3498

Skype: tractorcow

[Anselm Christophersen]

Which IDE are you referring to? I'm using phpstorm, and it doesn't seem to work there. Will have to double check though, to be completely sure.

Sent from my iPhone

[Anselm Christophersen]

This is what I meant:

Again, I might be missing something, but as long as ::create doesn’t allow me code hinting, I’ll likely stick with the old “new”.

This is on PhpStorm 8.0.1, and CMS & Framework on 3.1.x-dev

Anselm

[Will Morgan]

I have little knowledge of framework-specific autocomplete helpers, but don't Symfony, ZF2, etc have definitions built in to certain autocomplete engines?

From my time using Komodo with CodeIntel, there was the option to tick certain libraries and they would aid in your definitions.

So while I advocate using Injector / new X() over ::create, the end result would still be the same in that you could tell the IDE's code intel API what to expect when using one of the aforementioned syntaxes.

Kind of like this: http://community.activestate.com/forum-topic/creating-api-catalogs

--
You received this message because you are subscribed to a topic in the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/silverstripe-dev/IgOLurk87hc/unsubscribe.
To unsubscribe from this group and all its topics, send an email to silverstripe-d...@googlegroups.com.

To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Will Morgan

willmorgan.co.uk

[dam...@silverstripe.com]

I'm using netbeans. :) Funny, I thought PHPStorm detected phpdoc at least as well as netbeans did.

[dam...@silverstripe.com]

Although to be fair, netbeans doesn't support @return $this syntax yet, but it does support @return self (non late bound) and @return static (late bound).

See http://www.phpdoc.org/docs/latest/guides/types.html#keywords

[Conrad Dobbs]

Using PhpStorm 7.1.3, this recently started working for me on 3.1 sites.

[Matthew Bonner]

I tend to dislike magic methods, as it is often more effort to debug a magic method than it is to follow a series of method calls as it isn't always clear when a magic method is being called and not called and leads to overload issues if someone overloads a magic method when they shouldn't. In general, magic methods should be avoided in my view. Therefore, I'd prefer to see a general coding standard put in place whereby ::create is not just used for Object but for every class that can be instantiated.

[Matthew Bonner]

Anselm, this is an issue with PhpStorm, one of many, I am an avid user of PhpStorm, and sometimes you have to work with the quirks of the software. It certainly doesn't take much getting used to.

[Matthew Bonner]

Also, something else I've not mentioned is the ability to overload the magic _constructor signature's type hinting, if you create your own constructor method this is not possible, resulting in much cleaner and more logical code.

On Tuesday, September 23, 2014 2:59:31 PM UTC+1, Loz Calver wrote: