convert current CRUD wrappers in export.inc [#1612466]

Here is a patch that creates new procedural wrappers for all of the CRUD functions so far. I haven't touched the import/export methods yet as there haven't been implemented yet in the Exportable classes. I have removed any existing function that I don't think we need now as they have new wrappers (ctools_exportable_*) or now live in the classes.

I'm not sure if this is entirely what we want to do right now (removing so much), just trying to make the file more manageable whilst creating the new functions.

Comment	File	Size	Author
#15	1612466-15.patch	4.41 KB	damiankloip
#14	1612466-14.patch	4.44 KB	damiankloip
#13	1612466-13.patch	4.34 KB	damiankloip
#11	1612466-11.patch	4.34 KB	damiankloip
#8	1612466-8.patch	4.13 KB	damiankloip
#6	1612466-6.patch	3.69 KB	damiankloip
	ctools-exportables-crud-wrappers.patch	23 KB	damiankloip

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

damiankloip CreditAttribution: damiankloip commented 1 June 2012 at 10:11

Issue tags:

+VDC

tagging...

Comment #2

dawehner

German

CreditAttribution: dawehner commented 1 June 2012 at 21:19

+++ b/includes/export.incundefined
@@ -18,6 +18,24 @@ define('EXPORT_IN_DATABASE', 0x01);
+function ctools_exportable_new($exportable_type, $set_defaults = TRUE) {

Just wondering why it is called _new not _create, but sure it was named like that already before.

+++ b/includes/export.incundefined
@@ -70,6 +88,86 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+function ctools_exportable_save($exportable) {

the Drupal\ctools\ExportableInterface could be forced here, as well on the docu of the functions above.

Comment #3

merlinofchaos CreditAttribution: merlinofchaos commented 2 June 2012 at 16:34

We can add a _create to go with _new -- it's the problem of language when essentially merging multiple code paths.

Comment #4

damiankloip CreditAttribution: damiankloip commented 4 June 2012 at 11:48

There are already _new and _create function in this patch. At the moment _new just calls the create() method on the controller but assumes no values will be passed in. The _create function has a $values parameter.

I was going to save any type hinting for another issue. Makes sense to have a patch/issue that deals with this solely?

Comment #5

merlinofchaos CreditAttribution: merlinofchaos commented 6 June 2012 at 18:40

+function ctools_exportable_load_all($exportable_type, $reset = FALSE) {
+  // TODO: Implement $reset when this is in place.
+  return ctools_exportable_get_controller($exportable_type)->loadAll();
+}

We can remove reset entirely. That's an artifact of when we just used to use statics and is no longer needed this way.


+/**
+ * Change the status of a certain object.
+ *
+ * @param $exportable
+ *   The fully populated exportable object to enable.
+ * @param $status
+ *   The status, in this case, is whether or not it is 'disabled'.
+ */

Minor documentation mismatch

Also, actually removing the old _crud functions will break Views until we are able to update it to the new functions, so we should make that a separate patch.

Comment #6

damiankloip CreditAttribution: damiankloip commented 7 June 2012 at 06:35

File	Size
1612466-6.patch	3.69 KB

Ok, here is a new patch, taking into account the points in #5 and keeping all of the existing functions for now.

Comment #7

dawehner

German

CreditAttribution: dawehner commented 13 June 2012 at 07:31

Status:

Needs review

» Needs work

+++ b/includes/export.incundefined
@@ -28,8 +46,8 @@ define('EXPORT_IN_CODE', 0x02);
+  return ctools_exportable_get_controller($exportable_type)->create($values, $set_defaults);

As we change the signature shouldn't be change the docs of the function as well?

+++ b/includes/export.incundefined
@@ -70,6 +88,81 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ *   $object parameter contains values for any serial fields defined by the $table

another nitpick ... > 80 chars

Comment #8

damiankloip CreditAttribution: damiankloip commented 13 June 2012 at 09:16

Status:

Needs work

» Needs review

File	Size
1612466-8.patch	4.13 KB

Here is a new version that addresses dawehners points in #8, and adds type hinting to the CRUD functions

Comment #9

merlinofchaos CreditAttribution: merlinofchaos commented 13 June 2012 at 17:00

Status:

Needs review

» Reviewed & tested by the community

Looks good. I didn't nitpick it like dawehner though so perhaps he should nitpick it again to be safe.

Comment #10

aspilicious CreditAttribution: aspilicious commented 14 June 2012 at 08:27

Will leave this rtbc.

But... ;)

+++ b/includes/export.incundefined
@@ -18,18 +18,38 @@ define('EXPORT_IN_DATABASE', 0x01);
+ * Create a new object for the given $table.

CreateS, same for all the other functions.

+++ b/includes/export.incundefined
@@ -18,18 +18,38 @@ define('EXPORT_IN_DATABASE', 0x01);
+ * @param $exportable_type

@param string "I guess"

+++ b/includes/export.incundefined
@@ -18,18 +18,38 @@ define('EXPORT_IN_DATABASE', 0x01);
+ * @param $set_defaults

@param bool

+++ b/includes/export.incundefined
@@ -18,18 +18,38 @@ define('EXPORT_IN_DATABASE', 0x01);
+ *   If TRUE, which is the default, then default values will be retrieved
+ *   from schema fields and set on the object.

I wouldn't add the "which is the default" part. And what happens with False. the opposite? or something else?

+++ b/includes/export.incundefined
@@ -18,18 +18,38 @@ define('EXPORT_IN_DATABASE', 0x01);
+ * @return

Does this return a Drupal\ctools\ExportableInterface object?

+++ b/includes/export.incundefined
@@ -70,6 +90,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * @return

@return array

+++ b/includes/export.incundefined
@@ -70,6 +90,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * @return

@return int|bool (if SAVED_NEW is an integer)

Comment #11

damiankloip CreditAttribution: damiankloip commented 14 June 2012 at 09:09

File	Size
1612466-11.patch	4.34 KB

Thanks aspilicious, might as well quickly make those changes too :)

Comment #12

aspilicious CreditAttribution: aspilicious commented 14 June 2012 at 09:11

+++ b/includes/export.incundefined
@@ -70,6 +91,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * Load all exportable objects of a given type.

LoadS

+++ b/includes/export.incundefined
@@ -70,6 +91,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * @return

@return array

+++ b/includes/export.incundefined
@@ -70,6 +91,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * Save a single exportable object.

SaveS

+++ b/includes/export.incundefined
@@ -70,6 +91,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * Delete a single exportable object.

DeleteS

+++ b/includes/export.incundefined
@@ -70,6 +91,82 @@ function ctools_exportable_load_multiple($exportable_type, $ids = FALSE, $condit
+ * Change the status of a certain object.

ChangeS

Thats what I mean by "same for all the function ;)"