Closed (fixed)
Project:
Examples for Developers
Component:
Node Example
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Jul 2009 at 15:45 UTC
Updated:
6 Apr 2010 at 22:25 UTC
Jump to comment: Most recent file
Comments
Comment #1
jhodgdonJust a note that this example module is still in serious need of an update. However, updating it should probably wait until the Drupal 7 code freeze, as the APIs are in flux.
Comment #2
dale42Comment #3
dale42I have a working D7 version of node_example.module. Before I post my replacement candidate I'm hoping for answers to the following questions.
Q1: The Drupal 6 example (http://api.drupal.org/api/drupal/developer--examples--node_example.module) provided SQL for creating a database table required for the module. I've created a .install file and Schema API definition for the module (for my own testing sanity). Since using schema is best practice, I would like to include the .install file in the example. Comments/Objections?
Q2: If the Schema API is used, which database update method is better for the example: drupal_write_record() (Schema API) or db_insert()/db_update()?
I've used db_insert and db_update because they seem the best follow-on from the Drupal 6 example. However, I don't know if this is a situation where drupal_write_record() is best practice.
Comment #4
jhodgdonQ1: Definitely a .install file is a best practice!
Comment #5
Crell commentedSchema API is for defining the table structure. It's not for use in normal DB reads/writes. Technically db_write_record() is not part of Schema API, it's a layer above it.
Honestly, with db_insert(), db_update(), and db_merge() (don't forget about that one!), I'm not convinced that db_write_record() is even useful anymore. I'd stick to the direct query builders.
Comment #6
dale42Wasn't quite sure how to create this patch since the file is in /contributions/docs/developer/examples, so I created it in the examples directory.
I've tested out the module on a current version of Drupal 7 and didn't encountered any bugs (or more correctly, fixed any I encountered).
The one thing I'd like to raise is flagging the reader to the existence of the .info and .install files. Nothing is mentioned on the current API page. I put some text in the description header telling the reader about the files, but it doesn't give good direction on how they can get them.
Comment #7
dale42FYI: I left the theme function internal to the .module file to keep the example self-contained. Adding a tpl file adds to problem with the .info and .install files. If we solve that issue I think should do it as I believe it is best practice. Perhaps as an interim add a note explaining a tpl file is preferred?
Comment #9
tanoshimi commentedJust a quick note to say that this patch looks like an excellent node_example rewrite - thankyou.
Like many 'casual' module developers who do not follow every update on a day-to-day basis, when upgrading to D7 I have to look up information using a combination of the forum, the D6->D7 upgrade guide, coder module, and api.drupal.org. However, there is no substitute for a clear, concise example module that covers the main hooks. Had I found this thread earlier this week, it would have saved me a lot of searching for the correct syntax to use for revised hook_view when upgrading my module to D7! oh well.
Two points:
1.) I too have wondered about whether usage of drupal_write_record in D6 should be replaced by the db_insert syntax.. although both are "drupally" and supported in D7, it sounds like using db_insert is more likely to be future-proof (might drupal_write_record be dropped in D8?....). I'd like to see this clarified.
2.) Agree that adding a .tpl page might overcomplicate, but should the example include hook_theme and an associated theme_node_example_body or something similar? The limitation of the proposed D7 node_example_view is that it doesn't demonstrate usage of the new $build_mode parameter (which is one of the things that tripped me up!). I think many people currently use the $teaser parameter in D5/6 hook_view to switch between two themeable functions for teaser view and full node view - perhaps demonstrate somthing similar in node_example?
Comment #10
Crell commentedYes, use of db_insert(), db_update(), and db_merge() should be preferred over drupal_write_record(). The latter is still there only because we haven't gotten around to removing it yet, but I'd consider it vestigial and deprecated at this point*.
* I am aware that not everyone would agree with me in that belief. :-)
Comment #11
dave reidMoving to the new 'examples' modules project.
Comment #12
ilo commentedI've reworked the patch (actually a did start from scratch yesterday, and today I was pointed to this issue queue).
I've left the "theme" function for now, later, if we introduce a new example module to handle themeing examples we can remove the theme implementation in this module.
Comment #13
ilo commentedoh sorry, attached the wrong patch.
Comment #14
jhodgdonThe documentation in this patch definitely needs some clean-up.
But if the module works as written, I might be inclined to accept this patch for now, and then have a separate patch to clean up the documentation (not sure how the other maintainers of this project feel about that idea, either).
I haven't yet had time to test whether the module works with the current Drupal 7...
Comment #15
ilo commentedActually this is something that will happen.. I'm not an english natural speaker, and my english should deserve some review.
About the patches, I'll keep providing patches or more modules during the next days, and the patches will be focused on just "do the work". Later we can introduce better documentation for each function or functionality, as the language review should also be done.
About this patch, it did pass all test with the current CVS version of Drupal 7.
I would like to introduce some field API stuff here, but I'm planning to create a new module (just copy node_example as fieldapi_node_example e.g.) and replace $form stuff with fieldAPI code, allowing the translation of the color attribute. Another module that would be of interest is a theme_node_example, where we can improve the themeing functionality within a module, including a template file.
but.. one at a time :)
Comment #16
ilo commentedI was just finishing the nodeapi_example port to D7 and realised that the node_example_view() function is buggy (even if it works as expected. I've just reverted to use theme(), as I proposed to leave themeing issues to another module. This is how the function should look like:
Edited: php syntax
Comment #17
jhodgdonUmmm...
That last bit doesn't look right to me. Normally, #theme designates the theme function to be used, not fully-themed content (e.g. output of the theme() function). ???
Comment #18
ilo commentedsorry, it was not '#theme', it should be '#markup'..
Actually, the fix was to remove the "return $node;" sentence, and fix the "$build_mode = full", I've written right localy, but did a bad handfix into that comment.
sorry again. I would rerolled the patch, but I'm finishing the nodeapi_module and don't want to update local cvs copy untill a export the patch.
Comment #19
ilo commentedRerolled to fix the $build_mode issue, and the themeing.
Comment #20
ilo commentedrerolled for 7.x-1.x-dev
Comment #21
dave reidReminder: Please commit D7 patches to the HEAD CVS branch, not DRUPAL-7--1.
Comment #22
c960657 commentedShould be “Implement” instead of “Implements”. Several occurences.
In D7, the database schema is automatically installed before hook_install() is called.
This doesn't follow the usual coding style for hook_schema wrt intentation and wrapping.
Shouldn't the primary key be just vid?
Comment #23
ilo commentedMmm I guess I just was trusty on the original patch and didn't review the .install file. In fact, hook_perm doesn't exist in D7, replaced by hook_permission()..
Comment #24
c960657 commentedHook implementations now use type-hinting for $node (introduced in #595084: Rollback type hinting for $node).
Comment #25
ilo commentedMajor rewrite for these issues on the way..
Comment #26
c960657 commentedLooks like #595084: Rollback type hinting for $node may be rolled back.
Comment #27
ilo commentedThanks for both reviews c960657.
Comment #28
cyberswat commentedI had no idea this issue was here before creating #706738: Update the node_example for Drupal 7 I'm a little confused as to why anyone needs to use hook_schema now that we have fields.
Comment #29
cyberswat commentedClosed out my duplicated issue and moving the patch over here. This version creates the same content type that was previously created but does it completely with fields, adds an image to the content type, and uses a node view mode. There is no need to use the approach that was tekn in d6 to create this content type. I'm casting my vote that this example should be performed purely with fields. I had to add the view mode stuff in because you don't technically need anything in the module file to reproduce the functionality that was in the d6 example module.
Comment #30
cyberswat commentedComment #31
rfayThanks for the work on this, cyberswat. I'll try to take a look soon.
Comment #32
rfayHere's cyberswat's version, some minor changes, and a modest test added.
Comment #33
rfayThis passed the bot, although the bot didn't report back, so I'm committing. Thanks, all! @cyberswat, this is a great update.
Committed: http://drupal.org/cvs?commit=326852
Comment #34
tanoshimi commentedHi there,
I wondered if it would be possible for somebody to explain *why* the approach taken of creating a new content type using "purely with fields" is better than the previous D6 approach of storing all of the fields for each new content type together in a single mymodule_node table in the DB and then using hook_load etc. (or, alternatively, to make a comment on this post: http://drupal.org/node/713310)
I know that Fields are a Drupally-7 thing, and I really appreciate the example demonstrating how I can do it this way, but I guess as a module developer I'm looking for a compelling reason why I should change approach considering that the "old" way seems to work just as well if not slightly faster based on my very limited tests.
As the whole purpose of this module is to provide best practice for developers, I think that the explanation for design changes such as this (which I'm guessing will affect quite a few developers since this impacts any module that defines a custom node type) needs to be covered.
Thankyou for any guidance.
Comment #35
cyberswat commentedThe d7 version accomplishes everything that a module developer would do in the d6 version but allows drupal to provide the bulk of the work in it's api's. By properly defining the fields the d7 way you expose those fields in a standardized way that will let other modules reliably extend or work with the content types. In theory some module, like views for example, could read the field type definitions and widgets and know exactly how to display that field in a variety of ways. If another module needs to extend a content type it becomes easier because you can read a fields or entities definition and alter it. In reality it becomes easier for module developers to quickly implement new content types because they do not need to focus on every minute detail of the database definition and every hook involved in d6 to load or modify a nodes content. By using the fields approach the entire thing is done in a well managed .install file and all of the work you needed to do in the .module file via hooks is handled automatically by core. The fields approach helps each field type to be focused on independently so that it may be be optimized to work within best practices. This will ultimately provide a higher level of quality to the entire community because each module developer now has consistency for their content types provided, for the most part, by core. You get improved benefits in the database layer, the caching layer, the theming layer, and can really focus on your modules functionality vs it's data structure.
Take a look at http://api.drupal.org/api/function/hook_nodeapi/6 ... notice the lack of a tab leading to the documentation for the d7 version. Take a look at http://api.drupal.org/api/function/hook_load/7 and notice the changes in the loading process ... there are now references to entity_load() and field_attach_load() that did not exist in d6.
By using the view modes for the nodes developers are making the work of themers easier because everything is being handled through the Drupal API's before the node is ever rendered. This provides not only consistency that had previously been lacking, but opens the door for Drupal core to handle items like caching. If a node knows how to display itself via a view mode then having to constantly preprocess for customizations becomes less relevant.
I'm sure there are others that can provide a more elegant explanation ... I'll finish this one by stating that you are all but short circuiting the benefits of d7 by not taking this approach.
Comment #36
tanoshimi commentedThanks for the detailed reply. I accept your argument that e.g. Views could offer different presentations for different standardised widget types, and that generally speaking development is made better and more reliable if you do things the "Drupal way".
However, I'm not too sure about the "easier for module developers to implement new content types" argument: while there may be less code required in the .module file when you create new content types using the Fields API, you need significantly more code in the .install file. What's more, the resulting database schema seems a bit... ugly - I have a content type that has approximately 15 custom fields, which are currently stored as 15 columns of a single table. But using Fields UI, I need to have 15 tables, which just seems inefficient and makes database management difficult. The resulting SQL query required to simply retrieve and display a node takes much longer.
I guess that since I am creating modules for my own private use I don't care about making them easily customisable and themeable by other users - I just want to achieve a certain fixed functionality and for that it seems that hook_schema() and hook_load() is more efficient - all my theme functions are hard-coded in the module and this works great.
I'm sure that it's just my lack of understanding of the benefits of Field API - I'll do some more reading. Thanks again for your input.
Comment #37
cyberswat commentedMake sure to reference http://drupal.org/node/707832 if you would like additonal resources to help with the learning curve.
Comment #38
Crell commentedtanoshimi does have a point. While Field API is a better approach in most cases for Drupal 7, there are valid use cases for hook_load()-based approaches, they are still supported by core, and we should document somewhere how horrifically crappy that API is (DB structure and FAPI structure need to match up for no good reason whatsoever). :-)
Comment #39
ilo commentedSo clearly, a Field API example may provide "fieldable" content type, and we can leave this nodeapi example using the 'old way' of doing node attributes?
I'm not sure, but does 'examples for developers' module should worry about Drupal mainstream of doing things and just not cover specific cases?
Randy please, any direction on this? is there a place where to put these kind of argues for different aproaches? Due to the 'too many ways to do things with Drupal' I see not possible to cover all the cases..
Comment #40
jhodgdonI think that there should be one fields example and one non-fields (nodeapi) example. Just because we have fields available in core does not mean that there are not use cases for a module to define its own content type using node API.
Comment #41
rfayI agree with jhodgdon - We can make this one the node_with_fields example, and make another one based on the the earlier patch you posted, ilo.
Thanks!
Comment #43
cyberswat commentedGoing to keep this closed but just an fyi to the people on this issue that the concerns from @tanoshimi are open for discussion on #763724: Add a D6-style Node Example to the Node Example module ... I've learned a lot since this discussion and have a much more valid answer over there.
Comment #44
cyberswat commented