Port Driven API to Drupal 7

not in a while,
I'm lacking time,
sorry

PS: but I would review proposals if someone steps forward

BTW, are you interested on Driven API even without cdriven?

feel free to reopen #741548: Is there a D7 upgrade path?

or create a new if issue if you prefer (just drop a link on the closed one)

I'm re-openning this, there is an issue from WebChick on CD for it to be ported and used in D.O. #1007082: Port Comment Driven to Drupal 7

I'll start working on it mid Jan.

Changing Priority to match post in CD.

#1011382: Driven API internals

What are the plans here? :)

Wondering if I can help out here, but not sure if work has already been started by jm.federico sind mid jan is over...

We can also start working together in a shared git repo. I haven't looked at the code yet, just trying to help with issues related to http://drupal.org/node/1006924.

Sorry, things have been complicated, nothing has been done, any help is welcome!

Ok, I started this, haven't come very far.

Pushed my code to https://github.com/Berdir/driven. If someone wants to help work on that, please mention your github username here, then I can add you as a collaborator.

First commit https://github.com/Berdir/driven/commit/19311e7d7b3c03b7dcd729025656fd7f...

Found a few strange things already, maybe arhak can answer these (See @todos added in the above commit):

- Some crazy combinations with node_get_types(). Sometimes calling the function without using it and often you call it both with the default argument and 'names' but then only use the type and maybe the name. Note that node_get_types('names') (node_type_get_names() in D7) returns an array that is $key => $name. That should really be all you need.

- Mix of 'admin/content/node-type/' and 'admin/content/types/'. Bug? Search for 'admin/content' in the linked commit to see what I mean

- Theme functions are declared and implemented for D7 already?

- In the node_types_overview replacement page, which you only seem to be using when content.module does not exist, you are using a content.module theme function?

Also, It's quite nice that you already marked many D6 specific URL prefixes with @d6. Wouldn't it be easier to actually create a CONSTANT for both the settings prefix, node_types prefix and maybe others instead of a separate variable in different functions.

mention your github username here, then I
can add you as a collaborator.

I'm pretty much offline,
so I can't join you at github

[...] node_get_types(). Sometimes calling the function without using it [...]

I only found this on driven_menu (hook_menu),
probably introduced while refactoring the module

[...] and often you call it both with the default
argument and 'names' but then only use the type and maybe the name.

I only found this on driven_props_types_overview and _driven_props_types_overview_alter (both in driven_props.admin.inc file)
and it seems to be commented

  $types = node_get_types();
  // types should be sorted by their machine names #814726: links are shifted at content types' overview page (when having custom content types, e.g. panels_node)
  // since 3rd party's custom types would alter their order
  $names = node_get_types('names');
  ...
  foreach ($names as $machine_name => $human_readable_name) {
    $type = $types[$machine_name];
    ...
  }

same reason in both cases: to avoid foreach($types as ...)
the proper sort is achieved by means of: foreach($names as ...)

which for D7 means: node_type_get_types and node_type_get_names

(keep in mind that I'm not a savvy,
there might be shortcuts I never saw/knew)

Note that node_get_types('names') (node_type_get_names() in D7) returns an
array that is $key => $name. That should really be all you need.

indexes sort of a PHP array within a foreach matters
refer to bug report #814726: links are shifted at content types' overview page (when having custom content types, e.g. panels_node)

haven't reviewed how the sort should be for D7,
just answering why node_get_types('names') was introduced for D6,

thus, the machine names served for the iteration loop (ordering),
but the actual data comes from node_get_types('types')

notwithstanding, right now I'm reviewing _node_types_build (basically the same for D6/D7),
and both arrays (types and names) seems to be indexed the same...
but looking into the commit (at that time) it looks like:

   $types = node_get_types();
+  // types should be sorted by their machine names #814726: links are shifted at content types' overview page (when having custom content types, e.g. panels_node)
+  // since 3rd party's custom types would alter their order
+  $names = node_get_types('names');
   $row_index = -1;
   $header[3]['colspan']++;
-  foreach ($types as $key => $type) {
+  foreach ($names as $machine_name => $human_readable_name) {
+    $type = $types[$machine_name];

with the commit message:

bug fix #814726: links are shifted at content types' overview page (when having custom content types, e.g. panels_node): links are shifted in content types' overview page (sort should be by machine names)

- Mix of 'admin/content/node-type/' and 'admin/content/types/'. Bug?

in D6 admin/content/types is the content types overview
while admin/content/node-type/[type_url_str]/[local_task] is for local tasks under each content type
(you can check hook_menu of node module, i.e: node_menu)

now in D7 those would be:
- admin/structure/types (for admin/content/types in D6)
- admin/structure/types/manage/%node_type/[local_task] (for admin/content/node-type/[type_url_str]/[local_task] in D6)

- Theme functions are declared and implemented for D7 already?

I thought it would be helpful to minimize the migration trauma

that was done in the early stages of D7 development,
so don't trust me

- In the node_types_overview replacement page, which you only seem to be
using when content.module does not exist, you are using a content.module
theme function?

lol
is a bug introduced in the copycat process

but it is incipient, since not having such function registered causes an empty string,
thus, no harm, just dummy code

it should be just the return theme('table', $header, $rows); (without the theme('content_overview_links') tail)

#13. Ah, I got it, see http://api.drupal.org/api/drupal/modules--node--node.module/function/_no.... The names are sorted with "asort($_node_types->names);". So they have a different sort order than the types.

But the key of the names array is the type, and you don't use anything more than that in your code.
This is exactly the same:

foreach (node_type_get_names() as $type => $name) {
  echo $type;
}

$types = node_type_get_types();
foreach (node_type_get_names() as $type => $name) {
  echo $types[$type]->type;
}

The output is the same, in the same order. And the first one is much simpler :)

#14: Got it.

#15: Yes, the theme functions look ok for D7. But they can not work like this in D6, you can not write them in a way so that they work both for D6 and D7. Also, the hook_theme() implementation probably needs to be moved into the .module file, unless the .theme file is manually included because the function registry is no more.

#16: Yes, it causes no harm. I just wasn't 100% sure that there isn't a reason for this and I would have to look for a D7 replacement. Need to figure out anyway if the overriding of that page is still necessary.

@#17#13

The names are sorted with "asort($_node_types->names);".

thanks! I was quick-reading the code and didn't hit that asort

The output is the same, in the same order. And the first one is much simpler :)

indeed
I think that at the time I was looking for readability,
(note that inline code doesn't allow broken down comments)
and also minor impact on changing code (sometimes quick fixes are based on wrong assumptions without throughout review)

@#17#15

But they can not work like this in D6

but they do work

'arguments' => array('variables' => array())
is valid for D6, and whatever nonsense is assigned to 'variables' argument is actually ignored

moreover, on top of driven_diff.theme.inc you have

// it is aimed that every call to driven_diff_output
// can be replaced with a direct theme invocation
// therefore, its signature matches core's theme function
// (D7 fashion, the same as functions declared by this module)
function driven_diff_output($theme, $variables) {

unless the .theme file is manually included

it is always included (on top of the driven_diff.module)

because the function registry is no more.

I'm not aware (I saw that issue passing by, but haven't taken a look at it)
(and having it listed in the .info files[] doesn't make them available? I mean, isn't there a mechanism to autoload non-object functions?)

@#17#16

I just wasn't 100% sure that there isn't a reason for this

yes, of course,
and you're welcome to ask about anything

(actually, even after reading your comment, I looked at that line and didn't saw the nonsense tail, so I wouldn't find it by myself)

Need to figure out anyway if the overriding of that page is still necessary.

I think last time I checked (before D7-betas) it was needed,
(but not conditionally, since Fields are now in core)

PS: and if it is needed, most likely it would require a total rewrite

Ok, still learning what all these modules actually do.

but they do work

'arguments' => array('variables' => array())
is valid for D6, and whatever nonsense is assigned to 'variables' argument is actually ignored

moreover, on top of driven_diff.theme.inc you have

Oh, I see. Nice trick ;)

it is always included (on top of the driven_diff.module)

I see. I think it would actually be better performance-wise to move the hook-declaration into the .module file and don't always include the .theme.inc file. Then, Drupal core will automatically load the .theme.inc file when a corresponding theme function is used (and only then).

I'm not aware (I saw that issue passing by, but haven't taken a look at it)
(and having it listed in the .info files[] doesn't make them available? I mean, isn't there a mechanism to autoload non-object functions?)

No. It turned out that the function registry in many circumstances (especially with APC) actually decreased performance and it caused many issues when functions were added/removed (caches had to be cleared every time for example, bad DX). What still exists is the object registry. The result is that files[] declarations are only necessary for files which contain classes. Additionally, it is possible to declare so called groups of hooks, which can then be placed in a module.group.inc and module_implements() will look if these files exist and include them automatically. To keep the performance up, module_implements() is now cached too.

And of course, menu/theme callbacks can still be placed in other files too which are automatically loaded but they need to be defined explicitly, just like in D6.

I think last time I checked (before D7-betas) it was needed,
(but not conditionally, since Fields are now in core)

PS: and if it is needed, most likely it would require a total rewrite

Yes, most likely. I haven't even looked what your module does do that page, need to figure that out first :) Looking at http://api.drupal.org/api/drupal/modules--node--content_types.inc/functi..., it's now a renderable array, so it might be possible to inject something with hook_page_alter() instead of overriding the whole callback.

- Another thought I had when I looked at these property defining modules is that it actually looks very similar to what http://drupal.org/project/entity is doing. I'm wondering if that could be re-used somehow.

I think it would actually be better performance-wise

I think there have to be a lot of things that could be wiser (in many aspects)
and I got the first path that came to my mind (there was no team development, no debate)

to move the
hook-declaration into the .module file and don't always include

ah, now I get what you meant

yes, you're right, and that was done that way for laziness
(since I'm not a theming savvy, I thought I would be re-working theme functions constantly, that's why I put the hook at hand, and every theme related stuff in one separated file)

No. It turned out that the function registry [...]

thanks a lot for the sum up
now I know where that issue ended up

I haven't even looked what your module does do that
page, need to figure that out first :)

CCK (in D6) adds the "manage fields" operation between "edit" and "delete" (for each content type row)
so, what Driven API does is very similar, it aims to add the "driven properties" link,
but there is some tricky casuistic:
- whether CCK is there or not
- whether there are submodules that want to insert their tasks over there
-- if there is only one, then that link is directly put before "delete"
-- else (several submods want to insert their tasks), then a "driven properties" link is put, which will hold every sub-task as local-task (tabs)

so it might be possible to inject
something with hook_page_alter()

I guess the CCK experience brought that flexibility

it actually looks very similar to what
http://drupal.org/project/entity [2] is doing. I'm wondering if that
could be re-used somehow.

ooh, I'm not following that module,
so I have no clue about its scope (or approaches)

the mission of this module is to ease the DX of submodules (by any means one can find useful and common)
and particularly to meta-describe the properties, mainly to allow mapping them to a form/form_state and perhaps some hints to facilitate decision making (of operations performed over a form)

don't know whether that can be achieved relying on entity module

from entity module project description:

in order to provide a unified way to deal with entities and their properties.

this seems very interesting (and close to this module's goal)

@#23 hook_entity_property_info (and its alter) could be used to settle our meta-description in a common place,
but it seems that all FAPI related stuff would need to be added by this mod (since Entity API is CRUD oriented, but I don't see how it might be oriented to FAPI internals)

having it as a dependency might result in a counterproductive choice,
since that is a module under active development, and chasing a moving target for so little benefit might not be a good deal,
OTOH it can be the right way to standardization

the decision is up to you (I won't have the time to properly evaluate the fitness of entity module)

Ok, I managed to properly port the content type related part we were talking about to D7, commit is https://github.com/Berdir/driven/commit/4c4480f8dd5067213413b6aa630a36c0....

Not sure yet what to do re entity.module.

subcribe +1

subscribing...

Subscribe

Subscribe

subscribe

@#30
granted yukare and Berdir

Feel free to take my code and continue with that, I probably won't have time for it.

Subscribing...

subscribe

http://drupal.org/sandbox/tstoeckler/1093448

http://drupal.org/project/field_property

Here's another alternative to comment_driven that works in Drupal 7:

http://drupal.org/project/comment_alter

It can't do all the stuff that 'driven' can, but the most common use is for 'comment_driven'.