Inspired by how og_views handles this, I think it'd be nicer on a few different levels if each default view we provided lived in a separate file:

- Easier to find the code you're looking for

- Easier to document each view

- Easier to paste views export output since things don't have to be indented

- Easier to do CVS blame and generally do revision control archeology if the views are in separate files

CommentFileSizeAuthor
#1 460248-1.split_default_issue_views.patch161.62 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 460248-1.split_default_issue_views.patch161.62 KB

This patch does the following:

  • Creates a views/default_views subdir
  • Every default view lives in it's own [view_name].view.php file (sort of like .tpl.php -- we want .view to know it's just a default view, but we want .php to a) ensure that the webserver doesn't serve these pages to people who navigate directly to them and b) so that text editors/file system/etc knows they contain PHP code.
  • project_issue_views_default_views() just scans the subdirectory for every .view.php file, includes it, appends the view to the $views array, and finally returns the array.

Any objections?

aclight’s picture

aclight commented

It seems to me that this split would make exporting views even more of a pain, at least during development, as we'll need to either specifically check each project* view to see if we've changed it during development or re-export each of them separately. As it is currently, you just need to check all the appropriate views, export, and copy and paste into the default views file. This will also make creating patches that contain new views more difficult since you have to edit CVS/Entries manually to add the new file.

But certainly your other points are valid. What would be super handy is if the views_export module could export each view into a separate file of its own, giving it the appropriate name, in the appropriate directory.

I'm sort of on the fence about whether this is a good idea or not, but you're the one that's been doing the majority of the development recently, so if you think this is an improvement then I'm fine with it.

I didn't review the patch itself but what you described in #1 makes sense.

dww’s picture

dww
we/he/they
commented

A) Our default views are not just paste-over-able. There's some somewhat sketchy dynamic logic in there, e.g. to conditionally add filters for issue taxonomy terms for sites that run with comment_alter_taxonomy and issue tags, to dynamically do a bunch of things differently if search.module is enabled, etc. So, you can't just blanket export and paste, or you break things, anyway. I was more thinking about pasting sections of the views, or at least pasting new views if they don't do any dynamic stuff yet. ;)

B) Views tells you what you've changed, since the view is either "Disable" or "Revert". So, if you're changing things, you know exactly which ones you've changed anyway.

C) I'm not so concerned about adding new views via patches requiring new files. You don't need to manually edit CVS/Entries -- that's only for core where it's impossible to login as an authenticated CVS user. So long as you're authenticated at all, CVS will let you run "cvs add" (even if our ACLs prevent the corresponding "cvs commit"), and then you can see the new files with "cvs diff -upN".

Yeah, that would be a nice feature for views_export. ;) But, it wouldn't help us much, given how unholy our default views are (as per A).

aclight’s picture

aclight commented

Ah...I didn't realize we had conditional code in the default views, though that makes sense. So in that case, I guess splitting them up doesn't cause any new problems and solves some current problems, so sounds good to me.

hunmonk’s picture

hunmonk commented

the idea makes sense to me.

should that require be a require_once?

dww’s picture

dww
we/he/they
commented

Sure, require_once is fine.

Are we all in agreement with views/default_views as the location for these? the other subdirs in there are called "handlers" and "plugins". Should this one just be called "views", or is that more confusing?

Any other objections?

Thanks,
-Derek

aclight’s picture

aclight commented

I like views/default_views

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Committed to HEAD.

Status: Fixed » Closed (fixed)

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