Theme functions clean-up (and fix a fatal error when using Omega themes)

Thanks for the bug report! You're most likely posting in the right place :) If anything breaks with the Edit module installed, assume it's Edit module's fault — it's in alpha stage after all!

Please confirm that this works as well:
$var['title_attributes_array']['class'][] = 'edit-pseudofield edit-field edit-allowed edit-type-direct';

That should *add* Edit module's attributes, instead of overriding them, like in my original code and like in your proposed change.

I disagree. That's just overkill.

The way I proposed is also how e.g. Bartik does it:
$variables['title_attributes_array']['class'][] = 'element-invisible';

If this fixes the problem, I'll commit this ASAP. Please set to RTBC if it solves your problem :)

Regarding edit_node_wrap_(name|date): not doing it the way I currently do it implies that I have to pass an array into the theme function. Which means I have to do implode(' ', $var['classes']) in the theme function, which is bad. We don't want any logic in theme functions :)

Re: #3

I disagree that this should be a string of space-separated classes.

If this was a template file then template_process would convert the classes array into a string via drupal_attributes. I don't see the problem of using the same technique in a theme function.

For one thing, if someone wanted to adjust the classes they would need to work with the compound string, rather than an array.

Many (most?) theme functions use drupal_attributes for this very purpose. For example:

http://api.drupal.org/api/drupal/includes%21form.inc/function/theme_date/7
http://api.drupal.org/api/drupal/includes%21form.inc/function/theme_butt...
http://api.drupal.org/api/drupal/includes%21form.inc/function/theme_fiel...

In fact, looking at the code more closely, edit_preprocess_page does use drupal_attributes because it is creating a wrapper for the title using title_prefix and title_suffix. The code in edit_preprocess_node doesn't use drupal_attributes because array to string conversion is handled in template_process.

Either way, we really do need to using arrays for classes.

I also think we should be using $variables, rather than $var as it is more standard and readable.

I'm happy to create a patch if you agree with my comments.

You're absolutely right. I forgot about drupal_attributes() for a minute.

So then the classes defined in edit_preprocess_page() would need to be converted to use an array as well, right?

I also see your point about $var vs. $variables. I did that deliberately because I think it's too verbose and didn't feel like typing it so often back when this was still experimental code. I'm fine with converting this to Drupal core's conventions, but please do that in a separate issue/patch :)

So: yes, I agree with your comments, and greatly appreciate your offer for a patch! :) Unassigning myself. Thank you!

Thanks, Wim.

I'm working on cleaning up the documentation right now. I will work on the two above mentioned issues as separate patches once I have submitted the one I am working on now.

That's so awesome! :D Thanks :)

I wonder whether we should use this format instead of array_merge:

$variables['title_attributes_array']['class'][] = 'edit-pseudofield';
$variables['title_attributes_array']['class'][] = 'edit-field';
$variables['title_attributes_array']['class'][] = 'edit-allowed';
$variables['title_attributes_array']['class'][] = 'edit-type-direct';

This way, we can document each class if we want to and it avoids the slightly less readable array_merge syntax.

Fuller review:

+++ b/edit.moduleundefined
@@ -175,20 +175,20 @@ function edit_field_formatter_info_alter(&$info) {
+  if (array_key_exists('node', $variables) && entity_access('update', 'node', $variables['node'])) {

We should be using !empty($variables['node']) instead of array_keys_exist.

+++ b/edit.moduleundefined
@@ -175,20 +175,20 @@ function edit_field_formatter_info_alter(&$info) {
+      $variables['title_prefix']['edit']['#markup'] = '<div ' . drupal_attributes($attributes) . '">';

We don't need the space after div as drupal_attributes outputs that for us. We don't need the double quote either.

+++ b/edit.moduleundefined
@@ -210,30 +210,37 @@ function edit_preprocess_node(&$var) {
       'data-edit-entity-label'    => $i['bundles'][$bundle]['label'],

We shouldn't use shorthand variable names like $i. $entity_info would be much more descriptive.

+++ b/edit.moduleundefined
@@ -210,30 +210,37 @@ function edit_preprocess_node(&$var) {
+    $variables['classes_array'] += $entity_classes;

As mentioned in my above comment, I would prefer these to use the $variables['classes_array'][] = 'edit-entity'; syntax.

+++ b/edit.moduleundefined
@@ -210,30 +210,37 @@ function edit_preprocess_node(&$var) {
+      if (isset($variables['title_attributes_array']['class'])) {

This check can be avoided if we use the other syntax.

+++ b/edit.moduleundefined
@@ -301,12 +308,18 @@ function edit_preprocess_field(&$var) {
+    if (!array_key_exists('attributes_array', $variables)) {
+      $variables['attributes_array'] = array();

I much prefer if (empty($variables['attributes_array'])) { ... but it could be avoided altogether if using the other syntax for appending to the array.

+++ b/edit.moduleundefined
@@ -301,12 +308,18 @@ function edit_preprocess_field(&$var) {
+    $field_classes = array('edit-field', 'edit-allowed' ,"edit-type-$edit_ability");

The comma is in the wrong position in this line, but this will be gone if we switch to the array append syntax.

+++ b/includes/theme.incundefined
@@ -14,9 +14,10 @@
 function theme_edit_spanned_field($variables) {
...
   $output = '';

There is an unwanted newline here.

+++ b/includes/theme.incundefined
@@ -14,9 +14,10 @@
+  $output .= '<span class="' . implode(' ', (array) $variables['classes']) . '"';

As discussed, this should use drupal_attributes, rather than implode.

Let me know if you'd rather I picked up these issues. I'm keen to get on with it but don't want to step on your toes.

I have split the $variables changes out into a separate patch. This patch does the following:

• Changes $var to $variables throughout the module.
• Changes a couple of array_key_exists calls to use empty instead. This is recommended when you don't need to check for keys that may contain a value of NULL.
• Removed the space after the div and before drupal_attributes in edit_preprocess_page as drupal_attributes adds this for us. Also, removed a stray double quote from this line.

@Wim: are you going to be around on IRC today? I would love to have a chat with you about this.

This next patch deals with the original issue, that of the classes being strings rather than arrays.

Here is what I did:

• Changed theme_edit_spanned_field to use attributes array instead of the custom classes array. This brings it inline with core and allows us to put the data- attributes in there too, instead of concatenating strings as we were doing before.
• Uses the more verbose but easier to document syntax for appending classes to the attributes array.
• Adds the data- attributes to the attributes array in theme_edit_spanned_field instead of concatenating strings together.
• Generally cleaned up theme_edit_spanned_field to be more core-like.

This patch is stacked on top of the one in #13 for easier code review.

@psynaptic: I'm afraid not, I've ATM been calling >4 hours straight, it's almost 10 PM and I still haven't had dinner! Please shoot me an e-mail instead, if you want a reply by tomorrow.
(I can't be online tomorrow either, I'm having my graduation ceremony tomorrow afternoon, and I doubt you'll be online before noon in my timezone — which is Europe.)

Will look at these comments & patches tomorrow — thanks guys! :)

Thanks for the replies guys.

I'm actually in the UK, which last I checked was in Europe. ;)

I'll likely be online tomorrow looking to press forward with more on this. I'll keep my eye on my dashboard and look forward to speaking with you tomorrow.

#13

+++ b/edit.moduleundefined
@@ -175,20 +175,20 @@ function edit_field_formatter_info_alter(&$info) {
+  if (!empty($variables['node']) && entity_access('update', 'node', $variables['node'])) {

Won't this trigger a notice in case $variables['node'] does not exist?

+++ b/edit.moduleundefined
@@ -175,20 +175,20 @@ function edit_field_formatter_info_alter(&$info) {
+      $variables['title_prefix']['edit']['#markup'] = '<div' . drupal_attributes($attributes) . '>';

Nice clean-up, my ignorant mind didn't know about this :) Thanks!

#15

+++ b/edit.moduleundefined
@@ -233,22 +242,34 @@ function edit_preprocess_node(&$variables) {
-  $classes = 'edit-pseudofield edit-field edit-allowed edit-type-form';

This line, plus the identical one in edit_node_wrap_date() are a left-over from how it was written previously. This is a good clean-up :)

If you could answer/possibly solve the question for #13, I'd be happy to commit both patches. I'd already commit #15, but it depends on #13, so :)

Improved issue title.

Won't this trigger a notice in case $variables['node'] does not exist?

No, this is the point of using empty. It checks if the property is set and ensures a non-empty value.

From the PHP manual:

empty() is the opposite of (boolean) var, except that no warning is generated when the variable is not set.

Here is a re-roll of #13 as the other one doesn't apply any longer.

Here's a re-roll of #15 too, as it doesn't apply any longer.

@Wim: I didn't seem to get any attribution for my work in #1672598: Clean up inline code documentation in PHP files. I think that is because you edited my changes and didn't commit my patch directly using the workflow described in Adding a commit author. Would it be possible for you to please pass back any change requests to me so we can discuss the merits of each item and so that I may correct them myself? That way my patches can be committed directly and I'll get the attribution.

On a side note, I have added edit, fape, and every_field modules to drupalcontrib.org so we can get reference docs while we work. Cron needs to run but it shouldn't take long before they show up.

#22: Okay, thanks. Goddamn PHP mess: isset()/empty()/array_key_exists()…

#23/#24: I'll commit this tomorrow — thanks :)

#25: Actually I *did* give you attribution :) Please see http://drupalcode.org/project/edit.git/commit/8238c54 and http://drupal.org/node/1649212/committers :) AFAICT everything looks good?

#26: awesome :)

#25: Wow, sorry about that. I was looking at spark for some stupid reason!

Committed #23 & #24: http://drupalcode.org/project/edit.git/commit/06c4e96, http://drupalcode.org/project/edit.git/commit/70427a0.

I can confirm the patch is applied and works fine!

Excellent, thanks for testing resveld! :) If you have other feedback, feel free to create a new issue! :)

already done! ;)