The following article is severly outdated as Drupal 7 brings in PHP 5, and PHP class use for PDO and SimpleTest....
http://api.drupal.org/api/file/developer/topics/oop.html

CommentFileSizeAuthor
#7 326881.patch.txt12.45 KBjhodgdon
#7 oop.html_.txt16.33 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: Outdated » documentation
Category: task » bug

This is API documentation. Moving to correct issue queue.

webchick’s picture

Title: Object Oriented Design » Object Oriented Design documentation needs updating

Fixing title.

matt2000’s picture

I think the article still makes the good point that syntax does not OOP make. There is good OOP (conceptual) code that is not OOP syntax, and there is OOP syntax code that is NOT good OOP conceptually.

So, just +1 to update and not retire this article.

Nick Lewis’s picture

Status: Active » Needs review

I took a stab at rewriting it. Honestly, the argument holds almost no water now that Drupal 6.x and beyond allow PHP5 objects. Almost every argument made begs the question, "so why not just use proper objects? ". I think the article should be retired, and replaced with an article quickly explaining design decisions in the past (php 4.x), current efforts to bring objects into drupal, as well as big areas we think could benefit from a more object oriented approach.

Or better yet -- maybe just a broad overview/discussion of drupal's architecture at present [separate issue].

Posting this proposal for review. Forgive me if there's a way to make a patch that removes a file from certain versions but not others.

catch’s picture

Status: Needs review » Needs work

To retire it from HEAD, I'm pretty sure you'd just checkout HEAD, delete all the lines, make the patch, then commit it to HEAD - since it's impossible to delete files from CVS anyway. So marking CNW.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

I will take a stab at this.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.33 KB
12.45 KB

OK, here's my first pass at editing this document. It had some other minor issues (such as outdated references to hook_node_api() and non-conformance with the Doc Style Standards in a place or two), so I fixed those too (sorry for any confusion this may have induced).

In the process of updating the document (having now read it carefully), I have to agree with matt2000 in comment #3 above: I think it makes a good case, and I think it should stay on api.drupal.org.

The attached patch is for D7. I think it could also be used for D6, after removing the new paragraph that is specific to D7, and changing the hook references for node and taxonomy back to their D6 versions (hook_nodeapi and hook_taxonomy -- these hooks were split out into separate hooks in D7).

I have also attached the full HTML file, as I think it's best to review the whole document rather than just the changes.

I've given both a .txt extension, because apparently .html extension attachments are not allowed, and the patch file is in the contrib repository so it would fail auto testing.

matt2000’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

The only potential thing it, since this is more of a conceptual essay than actual documentation, it might be better to leave out references to specific functions, to minimize the need for further changes due to future release, for example,

+    <p>The above interaction is also similar to the use of observers
+    in object-oriented systems. This Observer pattern is pervasive
+    throughout Drupal, as many of Drupal's hooks essentially allow
+    modules to register as observers of Drupal's objects. For
+    instance, when a modification is made to a vocabulary in Drupal's
+    taxonomy system, a taxonomy hook such as
+    hook_taxonomy_vocabulary_update() is called in all modules that
+    implement it. By implementing the hook, the modules have
+    registered as observers of the vocabulary object; any changes to
+    it can then be acted on as is appropriate.</p>

But as far as Drupal 7 is concerned, this patch looks good to me.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This patch is a good first clean-up, but more work could be done. I'm OK with doing that in follow-up patches.

This isn't a patch against CVS HEAD though -- it doesn't apply to HEAD.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Needs work » Active

The patch applies fine for me in the Contrib repository on HEAD... of course, I created it... did I do something wrong?

As there were two "this is better than it was" reviews on the content, I went ahead and committed this patch to the contrib repository, with one change: I realized the D7 doc mentioned node_view() which doesn't exist, so I changed that sentence to say "node_build() followed by drupal_render()".

I also made the D6 changes (same patch applied, and then I went back and changed function references and removed the D7 paragraph saying "we're using PHP5 and some classes now), and committed it to the D6 contrib repository.

So I am setting this back to "Active" to get more comments on future changes that might be needed.

Dries: do you have specific comments on what type of work you would like to see done in follow-up patches?

And what does everyone think about removing all mentions of specific Drupal functions in the document? There are about 8 functions referenced in the doc as it is now, and I personally think that they are quite useful as examples, though it is true they make the doc harder to maintain. Thoughts?

jhodgdon’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: documentation » Correction/Clarification

This document is no longer in CVS -- see #547510: Move OOP document to handbook - it is now in the Handbook. Moving this issue to Documentation queue, accordingly.

I'm not sure what changes Dries has in mind, but leaving it open for now...

add1sun’s picture

So many dupes. Closed these others in favor of this one.

#554794: Drupal Programming from an Object-Oriented Perspective
#608174: Update and modernize our stance on OO

EDIT: ugh, wrong issue number there. Corrected.

Crell’s picture

So that's why that thread dropped off of my "My issues" page. Bah. For reference, here's what I posted in the other thread:

Currently, Drupal's quasi-official stance on object-oriented code is this handbook page: http://drupal.org/node/547518

The content in it dates from, oh, Drupal 4.5 or so when PHP 4.4 was still the hot new thing. It's outdated given that Drupal 7 requires PHP 5 which has a working OO model. It's inaccurate since in Drupal 7 we have a lot of genuinely OO code both in the DB layer and elsewhere. It's also not entirely true in places about what it says regarding OO patterns, and I know we do sometimes catch some flack for that. :-)

So since we've modernized Drupal 7 to a great extent, let's modernize our OO stance.

I volunteer to write the updated version, but we should briefly discuss what it should say. My general outline would be something like the following:

- Drupal is not fully OO. This is true, and partially historical.
- OO and procedural both viable approaches in PHP with different trade-offs. Discuss a bit.
- Places where Drupal recommends OO vs .places Drupal recommends procedural.
- Discussion of common design patterns, both OO and procedural, both generic and Drupal-specific. (See my Design Patterns talk in Paris for some background here.)

Thoughts? Feedback? Banana cream pies?

chx responded by pointing out that hooks are a form of pointcut: http://en.wikipedia.org/wiki/Pointcut I'd not heard of that before, but it sounds accurate.

Right now this is on my todo list, but performance patches for D7 are higher on the list. Anyone have further input?

jhodgdon’s picture

Status: Active » Closed (won't fix)

This issue is pretty old, and we aren't using Documentation issue reports to report problems with Community Documentation pages any more. I added a link to this issue as a comment on the page. Please feel free to edit the page...
http://drupal.org/node/547518
Meanwhile I've marked the page as "needs updating" and added a link to this issue in a comment.