Follows on from #1699750: move remaining functions out of flag.inc and remove it.

For my own sanity as a developer, I'd like the different classes in flag.inc to each be in their own file. Otherwise I find it gets really difficult to keep track of which method I'm looking at.

Comments

socketwench’s picture

Assigned: Unassigned » socketwench
Status: Active » Needs review
StatusFileSize
new118.14 KB

Yes, please! Here's a quick stab at it.

For now I dumped all of it into /includes, but perhaps we should consider creating another directory? /lib or /classes?

What about the classes in flag.rules.inc?

joachim’s picture

/includes is good; it's what most other modules use.

But I'm afraid this is going to get broken when #871064: Making flaggings fieldable lands. I should have said on this (and the other restructuring issues I've filed) that they're still sort of postponed. I just did a big brain dump yesterday to get these out of my head and into the issue queue. Sorry!

joachim’s picture

Hmm there's also all those cookie classes I'd forgotten about. I don't think those need a file each. Clearly needs more pondering :/

socketwench’s picture

But I'm afraid this is going to get broken when #871064: Making flaggings fieldable lands.

I was wondering that myself...

Hmm there's also all those cookie classes I'd forgotten about. I don't think those need a file each.

I'd probably stick those into their own directory within the "lib" or "classes" directory. It's more files, but it's better organized and maintains consistency.

socketwench’s picture

StatusFileSize
new126.15 KB

Now that #871064: Making flaggings fieldable is in, here's an updated patch.

joachim’s picture

StatusFileSize
new125.66 KB

Here's what I'm thinking:

- keep the cookies classes together in one file: you never use just one alone anyway
- put the flag classes in includes/flag. That's in line with the includes/views pattern (which we don't follow yet, but I'll file an issue for that ;) And also it helps to see which classes are flag types. And it prevents the ugly clash between flag_entity.inc and flag.entity.inc.
- put flag_broken at the bottom of flag_flag (same pattern as views classes again)

joachim’s picture

Also, this patch may need to ensure there's a clear of the class registry.

We possibly need to do a http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/registr... in an update function. Should cover all our class changes in other issues too.

socketwench’s picture

StatusFileSize
new126.1 KB

As much as I hate to give up on the one file = one class idea, if that's the Drupal standard, I acquiesce.

Added registry rebuild to install hook.

joachim’s picture

Status: Needs review » Needs work
+++ b/flag.install
@@ -436,3 +436,11 @@ function flag_update_7202() {
+ * http://drupal.org/node/1699752
...
+function flag_update_7204() {

Ah that should be 7300 because we're on the 3.x branch now.

There's also no need to link to this issue. Git blame can produce that if people want it :)

I'm going to test this with a flag 7-2 install (which I need to re-create, as I'd already started changing database tables on my localhost...)

joachim’s picture

Status: Needs work » Needs review
StatusFileSize
new126.03 KB

Here's the same patch with the update number changed and the comment tweaked.

I've tested in on a clean flag install and though applying the patch instantly breaks the site (as expected!), going to update.php makes it all work again.

Note: this same update function will cover us for moving the views classes, which I'll do once this is committed :)

socketwench’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Marking RTBC.

socketwench’s picture

And now do I catch your comment on Twitter. New rule: Tess does not get two coffees in the same morning; it scrambles her thought processes.

Committed to 7.x-3.x: http://drupalcode.org/project/flag.git/commit/53596dc

socketwench’s picture

Status: Reviewed & tested by the community » Fixed
joachim’s picture

One class per file is probably the PSR-0 way. So we'll be doing that for Drupal 8... :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.