New, consistent save routines [#260278]

Three modules -- three completely different _save() functions, even though the data structure is more or less identical. I think that's unnecessarily complicated and error-prone.
Even though we don't have drupal_write_record yet, at least we can make this consistent ("You know one, you know all").
I therefore completely refactored panels_page_save, panels_mini_save and panels_views_save. They are now as similar as possible, really easy to understand, and - according to my tests - work like a charm.

Some improvements for panels_mini:
- added panels_mini_fields() as a "poorman's schema" (just like panels_page_fields).
- checks if $panel_mini->display is not empty before calling panels_save_display(), to avoid an error.
- $panel_mini now gets properly sanitized, which wasn't the case before.
- panels_mini_fields() is also used by panels_mini_export().

Improvements for panels_page:
- much more straightforward building of the query string arrays with only one foreach construct.
- variables $fields and $types resp. $pairs and $values are easier to understand than $f, $q and $v. Also, they get initialized as arrays to avoid errors.

Improvements for panels_views:
- much more straightforward building of query string from arrays rather than from concatenated chunks.
- renamed $pv to $panel_view
- variables $fields and $types resp. $pairs and $values are even more obvious than $keys, $values and $args.

I also considered the alternative to use the panels_views poorman's schema as a blueprint for the two other modules, but then I thought it wouldn't give us enough value there and settled for panels_page_fields. I'm open to that though.

Would be awesome to create a class (just as panels_allowed_layouts) to give all those separate routines a home. That's another task though.

Comment	File	Size	Author
#5	new_save_routines_5.patch	11.27 KB	Pancho
#5
#3	new_save_routines_3.patch	11.26 KB	Pancho
#3
	new_save_routines.patch	11.24 KB	Pancho

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 19 May 2008 at 17:56

Status:

Active

» Needs review

Comment #2

sdboyer CreditAttribution: sdboyer commented 24 May 2008 at 05:53

So, with the number of changes that have been committed since you rolled this, the patch is thoroughly borked. Any chance you might be able to re-roll it? Sorry...

Comment #3

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 26 May 2008 at 22:04

File	Size
new_save_routines_3.patch	11.26 KB

Here's the requested reroll, needs to be thoroughly tested though, especially after these switcher_options got in (don't really know what they are about...).

Comment #4

sdboyer CreditAttribution: sdboyer commented 27 May 2008 at 21:47

Eeeek.

Sorry, but I committed your/sun's big code cleanup, so this needs rerolling. Again.

Comment #5

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 28 May 2008 at 12:55

File	Size
new_save_routines_5.patch	11.27 KB

No problem, the big code cleanup was a good thing to do before going RC.

Rerolled the patch - would be awesome to get it into the last beta, cause we probably don't want to do cleanups like this at a later stage...

Comment #6

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 3 June 2008 at 14:15

Component:

Code

» Attn -- s

Still applies. Would you mind taking a look at this one?

Comment #7

sdboyer CreditAttribution: sdboyer commented 4 June 2008 at 07:03

OK, here's my thinking on this. Consistent save routines = good idea. As far as my tests indicate, this works just fine. However, this is the type of thing I'd rather let sit on my machine through a release cycle while I do various crazy things in the course of patching & testing to help ensure that there really aren't any problems that will come back to bite us later. So, I think we should leave this out of Beta5, but give it as much testing as we possibly can so that it's ship-shape for RC. I'd be horrified if we had to release a 5a, let ALONE a 6, for any reason...

Comment #8

merlinofchaos CreditAttribution: merlinofchaos commented 9 July 2008 at 21:28

Status:

Needs review

» Needs work

+function panels_mini_fields() {
+  return array(
+    "name" => "'%s'",
+    "title" => "'%s'",
+    "contexts" => "'%s'",
+    "requiredcontexts" => "'%s'",
+    "relationships" => "'%s'",
+  );
+}

-  $fields = array('name', 'title', 'requiredcontexts', 'contexts', 'relationships');
+  $fields = panels_mini_fields();
   $output .= $prefix . '$mini = new stdClass()' . ";\n";
   $output .= $prefix . '$mini->pid = \'new\'' . ";\n";
   foreach ($fields as $field) {

I think that's the wrong array? You either need array_keys or $field => $type

Comment #9

merlinofchaos CreditAttribution: merlinofchaos commented 9 July 2008 at 21:29

FYI I agree with this patch, so let's prioritize this one.

Comment #10

merlinofchaos CreditAttribution: merlinofchaos commented 9 July 2008 at 21:34

Priority:

Normal

» Critical

Comment #11

sdboyer CreditAttribution: sdboyer commented 14 July 2008 at 05:09

Component:	Attn -- s	» Code
Status:	Needs work	» Needs review

@8 - yep, it's the wrong one. Fixed in my version.

I've run through and fixed these up, cleaned em up a little bit. I'm gonna commit this now, although I may add some more later by making panels_mini and panels_page use the same poor-man's schema API as panels_views does. There are also likely to be some quirks in it (though I've done a fair bit of testing), which is actually part of why I'm committing it now - I want it percolating in -dev as long as possible.

Marking it CNR since we need to keep an eye on it.