Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

Title: Using when a message referenced entity is deleted » Use Qeueu API when a message referenced entity is deleted
Status: Needs review » Needs work

Looks good.

+++ b/includes/message.admin.incundefined
@@ -105,6 +105,13 @@ function message_user_admin_settings($form_state) {
+  $form['message_entity_delete_use_queue'] = array(

Lets call the variable message_use_queue. (The queue name should be message_entity_delete)

+++ b/includes/message.admin.incundefined
@@ -105,6 +105,13 @@ function message_user_admin_settings($form_state) {
+    '#description' => t('Delete messages with out existing reference via queue.'),

with out => without

+++ b/message.moduleundefined
@@ -271,6 +271,29 @@ function message_entity_delete($entity, $entity_type) {
+  if (!$fields = _message_get_referenced_fields($entity, $entity_type)) {

$fields => $field_names

+++ b/message.moduleundefined
@@ -271,6 +271,29 @@ function message_entity_delete($entity, $entity_type) {
+function _message_get_referenced_fields($entity, $entity_type) {

missing @params. Also the entity-type should be first.

+++ b/message.moduleundefined
@@ -319,18 +335,56 @@ function message_entity_delete($entity, $entity_type) {
+ * Delete messages that the reference two is still exists.

Add line break.

+++ b/message.moduleundefined
@@ -319,18 +335,56 @@ function message_entity_delete($entity, $entity_type) {
+ *  The number of message to process each time. Optional.

Optional; The number ...

+++ b/message.moduleundefined
@@ -319,18 +335,56 @@ function message_entity_delete($entity, $entity_type) {
+ * @return The last Message ID we process.

@return
The last ...

RoySegall’s picture

I changed the name of $fields to $fields_info(i think this is would be more appropriate). Also the entity type is the second argument - you can see it in hook_entity_delete

amitaibu’s picture

Status: Needs work » Needs review
amitaibu’s picture

Category: bug » feature
Status: Needs review » Needs work
+++ b/message.moduleundefined
@@ -319,18 +340,58 @@ function message_entity_delete($entity, $entity_type) {
+    $fields[] = array(
+      'field_name' => $field['field_name'],
+      'column' => $column,
+      'cardinality' => $field['cardinality'],
+      'id' => $id,

We don't need to keep this fields' info -- enough to keep the field name, and when needed field_info_field() to get the relevant data.

(so you can change the variable to $field_names after all).

RoySegall’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Apply changes.

Status: Needs review » Needs work

The last submitted patch, message-reference-delete-in-batches-1945144-5.patch, failed testing.

amitaibu’s picture

Tests are failing. Also:

+++ b/message.moduleundefined
@@ -265,12 +265,40 @@ function message_cron() {
+  if (!$field_names = _message_get_referenced_fields($entity, $entity_type)) {
...
+function _message_get_referenced_fields($entity, $entity_type) {

Let's change it to ($entity-type, $entity). The fact its ike this in hook_entity_delete() was fixed in D8.

+++ b/message.moduleundefined
@@ -278,14 +306,7 @@ function message_entity_delete($entity, $entity_type) {
+  $fields = array();

$fields => $field_names

+++ b/message.moduleundefined
@@ -319,18 +340,54 @@ function message_entity_delete($entity, $entity_type) {
+ * Delete messages that the reference two is still exists.

reference two is still => reference to still

RoySegall’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

The hook entity delete get first the entity object and then the entity type although the entity delete function get the entity ID and the entity type second(confusing indeed).

Any way, i make the changes(except for the order of the hook entity delete arguments).

RoySegall’s picture

Status: Needs review » Needs work

The last submitted patch, message-reference-delete-in-batches-1945144-9.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
+++ b/message.module
@@ -265,12 +265,45 @@ function message_cron() {
+      'last_mid' => 1,

This should probably 0, otherwise you might lose that one.

+++ b/message.module
@@ -381,7 +443,10 @@ function message_entity_delete($entity, $entity_type) {
+  return;

Is that needed?

+++ b/message.module
@@ -930,3 +995,38 @@ function message_features_pipe_message_type_alter(&$pipe, $data, $export) {
+function message_entity_delete_queue_worker($data, $end_time = FALSE) {

You can remove the $end_time

RoySegall’s picture

Status: Needs review » Needs work

The last submitted patch, message-reference-delete-in-batches-1945144-12.patch, failed testing.

amitaibu’s picture

Seems there's a notice.

RoySegall’s picture

I found the problem - i didn't store the column in the field schema and didn't store the referenced entity identifier that were deleted. I think those are important data ;)

RoySegall’s picture

Status: Needs work » Needs review
RoySegall’s picture

I'm adding another patch that prevent a minor problem but an important one: for every deleted entity a queue worker will be created no matter of there are any messages referenced to that entity.

Patch is attached.

Status: Needs review » Needs work

The last submitted patch, message-reference-delete-in-batches-1945144-16.patch, failed testing.

RoySegall’s picture

Status: Needs work » Needs review
FileSize
8.79 KB

Status: Needs review » Needs work

The last submitted patch, message-reference-delete-in-batches-1945144-19.patch, failed testing.

RoySegall’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
amitaibu’s picture

Shouldn't this patch change also message_cron()?

amitaibu’s picture

Issue summary: View changes
Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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