Variables of the same type/meaning naming is not consistent in the API. Opening this issue to track them down and fix them.

It's better to have a poorly named variable but using a consistent name accross the framework than having 10 good names for the same things and always switching from one to another.

CommentFileSizeAuthor
#17 better-variables-names-2.patch8.17 KBpounard
#15 better-variables-names.patch8.03 KBpounard
#9 context-to-contextvalues.patch3.49 KBpounard
#8 offset-to-contextkey.patch6.85 KBpounard

Comments

pounard’s picture

pounard
Primary language French
commented

First one spotted: in DrupalContext::registerHandler() first parameter name is $context: it should be $offset to be consistent with offsetExists() and offsetGet() methods.

All over the API all variables that contains a "context key" should be called $offset (or $offset should be changed to something else everywhere, including ArrayAccess methods).

$context may be reserved to DrupalContextInterface instances.

Crell’s picture

Crell commented

If we end up dropping ArrayAccess, then all $offset references should probably fold to $context_key.

I agree with reserving $context to context objects.

pounard’s picture

pounard
Primary language French
commented

Potential second one: the $context property of DrupalContext is not a context, but context data, may be something like $contextData or just $data could be better?

Crell’s picture

Crell commented

$data is a terrible variable name, as all variables are data. :-) But yes, it's a poor name. $contextValues, perhaps?

pounard’s picture

pounard
Primary language French
commented

$contextValues sounds good!

Crell’s picture

Crell commented

Patch welcome.

pounard’s picture

pounard
Primary language French
commented

Will do, don't worry, everything at its time.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new offset-to-contextkey.patch6.85 KB

Fixes #1 $offset removal.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new context-to-contextvalues.patch3.49 KB

Fixes #4 $context to $contextValues

pounard’s picture

pounard
Primary language French
commented
Status: Active » Needs review
Crell’s picture

Crell commented
Status: Needs review » Needs work

Those are not going to both commit as patches, since they affect the same lines. Can you make a branch that has both of those as separate commits, one after the other? I can merge that in fairly easily, I think. Git is smarter than patch. :-)

Also, $contextKey is not a valid variable name. It's not an object property, so should be $context_key. $contextValues is fine as that's an object property.

pounard’s picture

pounard
Primary language French
commented

Why do you stick with names with _ while the whole code is camelCased? I sincerely think it mixes multiple conventions and is confusing.

Crell’s picture

Crell commented

Drupal's coding standards say non-property variables are $lower_case_named. Until/unless that changes, that's what we use.

pounard’s picture

pounard
Primary language French
commented

I will respect the convention, but it does not prevent me from arguing them.

By the way http://drupal.org/node/608152 does not include any comment about variable in OOP code. While http://drupal.org/coding-standards remains strongly procedural oriented: I think there's some void arround the topic which makes it arguable.

pounard’s picture

pounard
Primary language French
commented
Status: Needs work » Needs review
StatusFileSize
new better-variables-names.patch8.03 KB

Here is a patch, fixes:
1. Some $context to $context_key (Context::registerHandler() signature)
2. $offset removal to $context_key
3. Context::$context to Context::$contextValues

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/includes/Drupal/Context/Context.php
@@ -101,102 +101,102 @@ class Context implements ContextInterface {
           // Lazzy handler instanciation.

Proper spelling: Lazy instantiation. :-)

+++ b/includes/Drupal/Context/Context.php
@@ -101,102 +101,102 @@ class Context implements ContextInterface {
-          if (isset($this->handlers[$context_key])) {
-            $handlerValue = $this->handlers[$context_key]->getValue($args);
+          if (isset($this->handlers[$local_key])) {
+            $handlerValue = $this->handlers[$local_key]->getValue($args);
             // NULL value here means the context pass, and let potential parent
             // overrides happen.
             if (NULL !== $handlerValue) {
               // The null object here means it's definitely a NULL and parent
               // cannot override it.
               if ($handlerValue instanceof OffsetIsNull) {
-                $this->context[$offset] = NULL;
+                $this->contextValues[$context_key] = NULL;
               } else {
-                $this->context[$offset] = $handlerValue;
+                $this->contextValues[$context_key] = $handlerValue;

$handlerValue should be $handler_value.

Otherwise looks good.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new better-variables-names-2.patch8.17 KB

Fixed and pushed in the 1290476-variables-naming new branch.

pounard’s picture

pounard
Primary language French
commented
Status: Needs work » Needs review
Crell’s picture

Crell commented
Status: Needs review » Fixed

Thanks, pounard. Branch has been merged.

Status: Fixed » Closed (fixed)

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