As discussed in #1100198: Context caching causes clearing problems, comments from #31 to #38

Changing the:
HandlerInterface::getValue(array $args = array())
method signature to:
Handler::Interface::getValue(array $args = array(), ContextInterface $context = null)

Also remove the ContextInterface parameter from the HandlerAbstract::__construct() will allow us to:

  • Share handler instances to multiple contextes: potential memory save
  • Make consistent HandlerInterface, HandlerAbstract and context handler manipulation altogether
  • Remove circular dependency between Context and HandlerAbtract (because HandlerInterface is not actually dependent on ContextInterface)
  • Implement later handler inheritance without spawning tons of objects
CommentFileSizeAuthor
#7 handler-flyweight-2.patch6.17 KBpounard
#1 handler-flyweight.patch6.35 KBpounard

Comments

pounard’s picture

pounard
Primary language French
commented
Status: Active » Needs review
StatusFileSize
new handler-flyweight.patch6.35 KB

Notice: this patch is dependent of http://drupal.org/node/1290476#comment-5064832

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/includes/Drupal/Context/Handler/HandlerAbstract.php
@@ -2,7 +2,7 @@
-use \Drupal\Context\ContextInterface;
+use Drupal\Context\ContextInterface;

Why remove the leading \? That's not relevant to this patch.

I think this looks sane otherwise, but holding on #1290476-16: METAISSUE - Param and variable name consistency since it's a dependency.

pounard’s picture

pounard
Primary language French
commented

Doing it by reflex and convention: Zend 2 and Symfony also does it. By the PHP error messages in error log I can safely assume that internally the leading slash does not exists: there is no notion of relative namespace.

I always leave the leading slash on SPL stuff only, also by convention: identify those directly. There probably is a technical reason of doing it since Zend 2 does it a lot in git code I read. I guess that Sympfony prefers to use \SomeClass directly in their code:, IMHO it's best to declaring all dependencies using the use statement. Seems a good thing to do: it creates a natural dependency index in your file header and the developers can see all of them at once at first sight when they open the file.

These conventions are extensively good for DX IMHO: it's a discussion that should happen for core and whatever is the result should lead to a coding standard modification.

Crell’s picture

Crell commented

The leading \ on global classes is to ensure that the autoloader doesn't try to find it in the current namespace, I believe. That's a performance question.

We should probably open another issue in core for namespace usage. In general, absent an existing standard we should probably follow Symfony's lead unless there's a specific reason not to since that way we're more consistent. I'll do that in a little bit.

pounard’s picture

pounard
Primary language French
commented

From you:

The leading slash on global classes is to ensure that the autoloader doesn't try to find it in the current namespace, I believe. That's a performance question.

From http://php.net/manual/en/language.namespaces.rules.php

Calls to fully qualified functions, classes or constants are resolved at compile-time. For instance new \A\B resolves to class A\B.

[...]

All unqualified and qualified names (not fully qualified names) are translated during compilation according to current import rules. For example, if the namespace A\B\C is imported as C, a call to C\D\e() is translated to A\B\C\D\e().

[...]

This is an identifier with a namespace separator that begins with a namespace separator, such as \Foo\Bar. namespace\Foo is also a fully qualified name.

Fully qualified names, for all objects are resolved at compile time (a fully qualified name is a name using a leading slash).

As soon as you have import rules (the use statement) your names are resolved at compile time, which means you have no impact at all at runtime.

However,

Inside namespace (say A\B), calls to unqualified functions are resolved at run-time. Here is how a call to function foo() is resolved:
It looks for a function from the current namespace: A\B\foo().
It tries to find and call the global function foo().

[...]

What I read here (but I can misinterpret it) is that if you use new Some\Class; somewhere without importing it, you delay the class name resolution at runtime: this has a performance impact on runtime. Everything else you imported using the use statement will be resolved at compile time.

Autoloading cannot suffer from the lack of leading slash, if you read well the first comment on the PHP manual page, every resolution attempt (at compile or at runtime) is relative either to global either to the specific namespace you are and end up by one possibility only.

So my conclusion is:

In all cases, it cannot impact autoloading at all, only name resolution before autoloading.

As soon as you use the use statement, resolution is done at compile time, which makes no difference at all at runtime, I guess especially when using an OPcode cache.

I will ask some questions on StackOverflow about this, it's an interesting topic, but to answer your question, no, it doesn't affect performances (except for compile time which basically is saved thanks to OPcodes, and that seems really insignifiant anyway).

No, it's only a matter of standard. Personally, I'm lazy and the slash does not have a nice finger position on french keyboard: I'd be glad to save a slash to type.

The only case where it matters is for SPL classes, because they basically are really common name (potential conflicts and your namespace will override SPL) and they all are in global scope.

pounard’s picture

pounard
Primary language French
commented

You are right about opening a core issue for this anyway. I will restore the leading slash for this patch in particular. I cannot promise you doing it before a few day though.

pounard’s picture

pounard
Primary language French
commented
Status: Needs work » Needs review
StatusFileSize
new handler-flyweight-2.patch6.17 KB

Fixed and pushed in 1297114-handlers-flyweight branch.
This branch was branched upon the 1290476-variables-naming: the merge will be seamless.

Crell’s picture

Crell commented
Status: Needs review » Fixed

Branch merged. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Typo