Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete data when basic data streams are deleted. Fixes #488 #489

Merged
merged 3 commits into from Jan 19, 2022

Conversation

mstenta
Copy link
Member

@mstenta mstenta commented Jan 16, 2022

See #488

Copy link
Member

@paul121 paul121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we had this when we had a separate legacy data stream bundle, but the hook_data_stream_delete implementation got dropped with this commit: e528813

We should add a test for this so we don't forget it in the future.

This PR is nearly identical to the original code though, so LGTM otherwise!

function farm_sensor_listener_data_stream_delete(DataStreamInterface $data_stream) {

  // Remove all legacy listener data provided by the data stream.
  if ($data_stream->bundle() == 'legacy_listener') {
    \Drupal::database()->delete('data_stream_legacy')
      ->condition('id', $data_stream->id())
      ->execute();
  }
}

@paul121
Copy link
Member

paul121 commented Jan 18, 2022

Added a test for this!

In the process, noticed that we have the logic for removing references to data streams in the wrong place, it should be in farm_sensor. Moved the logic and added another test for this, but running into the entity reference integrity issue... so I marked the test to be skipped for now, but this still causes our test runner to fail. Gotta run but wanted to push here for now...

@mstenta
Copy link
Member Author

mstenta commented Jan 19, 2022

Thanks @paul121! I separated this into two branches...

  1. 2.x-delete-data-stream-data with my original commit + your test for deleting data
  2. 2.x-data-stream-references with your two commits re: asset reference fields

I'll point to (2) from #485...

Running tests on this PR (2.x-delete-data-stream-data) now... I will merge if it passes.

@mstenta
Copy link
Member Author

mstenta commented Jan 19, 2022

Ah tests failed because SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "test36388637asset__data_stream" does not exist - this is probably what you ran into, huh @paul121 :-)

Whelp... I suppose we can just delete that original code from hook_ENTITY_TYPE_delete() (which is causing the test to fail), since we discovered in #485 that it isn't working as intended anyway (and maybe we'll end up taking a different approach to solve that issue, removing the need for that code).

@mstenta mstenta merged commit 2aac4f1 into farmOS:2.x Jan 19, 2022
@mstenta mstenta deleted the 2.x-delete-data-stream-data branch January 19, 2022 03:22
@paul121
Copy link
Member

paul121 commented Jan 19, 2022

Precisely - that's perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants