Change the filter in the details page for migration messages to a condition object

Changed the migration message filter to a condition object.

Even when the code seems similar to dblog, this is a migration system related code. Moving to the right component. Maybe some tests?

@dagmar: There is already testing for this, see: Drupal\Tests\migrate\Functional\MigrateMessageFormTest.

@dagmar: Thanks for fixing the component. I think I cloned the other issue and forgot to update that.

I agree with @daffie in #6: we are refactoring the code, so the existing test coverage should be enough.

I think there is room for simplification, so back to NW. See my comments on the MR.

@benjifisher: Thank you for the great review. I think I have addressed all your remarks.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Everything is green.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I think the testbot is doing a test merge of the feature branch from the MR into 11.x, and it finds merge conflicts. I do not have time right now to confirm that.

I did notice, when I was reviewing, that another issue made the same changes to the performance test as this issue. That may or may not be the cause of the conflict. If not, then the MR should probably be updated to increment those two counts again.

@benjifisher: I think we have 1 open remark left and that is the one for not passing the database query by reference. As you are one of the subsystem maintainer of the migrate system, you may decide how you want this solved. Could you give a code example how you would like it solved, then I will implement it that way. I have now: $this->addFilterToQuery($request, $query);

@daffie:

You do not have to change how the method is called. Just change the declaration: instead of

  protected function addFilterToQuery(Request $request, SelectInterface &$query): void {

make it

  protected function addFilterToQuery(Request $request, SelectInterface $query): void {

Reminder to self: remember the T in RTBC.

I notice that the current MR does not change the performance test. I notice that 📌 Remove cache tag checksum assertions from performance tests Active is now RTBC, so maybe we will not run into that problem in the future.

I have removed the by reference part of the $query parameter. The CI pipeline stays green.

@benjifisher: Could you maybe explain why the by reference is not needed the $query parameter. We are changing the query in the method and we need those changes. Only it works without the by reference part. ???

@daffie:

I am sorry to be so slow in responding.

It is kind of crazy, but true: as I said on the MR, "you almost never have to pass an object by reference." It is pretty well explained in the PHP manual. From the Introduction to Classes and Objects page (https://www.php.net/manual/en/oop5.intro.php):

PHP treats objects in the same way as references or handles, meaning that each variable contains an object reference rather than a copy of the entire object. See Objects and References

Follow that link to see an explanation and some examples.

I will have a look now at the updates to the MR.

I made 3 more comments on the MR (and resolved all the existing threads). Two are minor, but using an AND condition group instead of an OR condition group is important.

I also considered whether the user input is handled securely. It is: all session data ends up inside $condition_or->condition(...), so it should be safe.

I still need to do some manual testing once the AND condition is restored.

I am sorry to be so slow in responding.

@benjifisher: No worries, I am very happy that you review.

I have addressed all remarks from @benjifisher.

@daffie:

Even better: you removed the condition group completely. Good idea, I wish I had thought of it.

The code looks great. I have resolved all the threads on the MR.

Testing

I hacked core/modules/node/migrations/d7_node_complete.yml to generate some messages:

title:
    - plugin: log
      source: title

I have a D7 site that I use for migration testing, with 100 nodes from devel_generate. On my D11 site, I enabled migrate_drupal_ui and imported the content from that site.

Then another hack: I used SQL queries to change the level of several messages (10 each for level 2, 3, 4).

After those hacks, I tested the filters: filtering on one or two levels (severity), filtering on "qu" in the message, filtering on both the level and the message. Everything worked as expected.

@benjifisher: Thank you for reviewing the issue. It is very much appreciated.