Start using OOP for flags
| Project: | Flag |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Jump to:
One of the problems with our current code is that some sections of it, mainly those dealing with the differences between nodes and comments, are "sphagetti code".
OOP can help us solve, or mitigate, this maintainability problem. Fortunately, our code already uses a $flag object, so extending this object to do more work for us isn't hard.
This patch is a proposed base for our object oriented flags. The support here is for CRUD and for the handling of the settings form only. We will gradually move more bits into this flag object.
The code mimics the style of Views 2. Programmers familiar with Views will recognize several patterns here. We're going to borrow still more wizdom from Views.
The code, 'file.inc', is easy to read, I believe. It's documented. The intention is that 'flag.module' will eventually contain mostly 'glue' code.
A short overview:
A flag is represented by a class. Our class hierarchy is:
flag_flag (abstract)
|
+--flag_node (represents node flags)
|
+--flag_comment (represents comment flags)
|
+--flag_user (represents user flags; future)Two common terms used in the Views 2 world are "definition" and "handler". A handler corresponds to the class that represents a flag. A definition tells our module about the existence of this class.
| Attachment | Size |
|---|---|
| flag.inc_.txt | 10.18 KB |
| flag.module.diff | 14.36 KB |
#1
I had an philosophical discussion with Eaton and Merlin about benefits and detractors from this approach. In the end, the benefits are many:
- CRUD functions (yay! nice)
- Better error handling
- Cleaner implementation in the module itself
- Little change in memory footprint, no additional data is actually being stored anywhere.
The only downside I can think of, this kind of coding isn't yet widely understood or utilized by the Drupal community.
Personally, I decided I don't have a problems with this. Let's shape the future :)
On implementation:
- Nice loader function in flag_get_flags(). No additional SQL overhead.
- In flag_get_flags(), I made a mistake at one point and uses the $fid as the key in the returned array. I'd like to change this to using the flag name instead. We can save it for a later patch though.
So, bottom line. Looks great! I haven't tested yet. Let's put out beta3 as a marker pre-oop. And then put this in.
#2
I'm glad you're ok with this.
Yeah. Today I fixed and closed two bugs ('actions' related), and our issue page indicates there aren't more open bugs. (Well, there's a http 200 error which we haven't figured out yet, and there's one javascript cleanup which I marked as 'bug' but is really a 'task'.)
So it seems nothing stands in the way of beta3, doesn't it?
#3
Beta3 is now released. I'm mostly taking the holiday off (it's Independence Day here) and I'll be leading training all next week. I'd like to have this tested out with the Views and Actions support before committing (basically try to break as few things as possible :). Whenever you feel like it's ready, put it in!
#4
A great day, yes.
I've been working on the Actions bugs and participated in the last V2 discussion with that OOP in place, so I know I didn't break anything. And it can't really break things: this specific OOP patch affects very little: CRUD and the settings form only. All else wasn't touched. I'm changing the subject of this issue to "start using oop", instead of "use oop", to show that this patch is only the beginning.
Thanks. I'm excited about this patch: I believe this OOP will make our code much easier to maintain. I'll soon commit to D6. Then to D5.
#5
Commited.
http://drupal.org/cvs?commit=125668
http://drupal.org/cvs?commit=125682
#6
Automatically closed -- issue fixed for two weeks with no activity.