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

(REF) Queues - Expand docblocks. Extract SQL trait. #22681

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

totten
Copy link
Member

@totten totten commented Feb 2, 2022

Overview

A few misc cleanups in CRM/Queue/*.

Before

  • Worse docblocks
  • More duplicate code (between CRM_Queue_Queue_Sql and CRM_Queue_Queue_SqlParallel)

After

  • Better docblocks
  • Less duplicate code (with CRM_Queue_Queue_SqlTrait)

Comments

There is pre-existing unit-test coverage for the refactored classes/methods (by way of tests/phpunit/CRM/Queue/). There is even some E2E coverage by way of civicrm-upgrade-test.

@civibot
Copy link

civibot bot commented Feb 2, 2022

(Standard links)

@civibot civibot bot added the master label Feb 2, 2022
@totten totten force-pushed the master-queue-cleanup branch from b28ab68 to 9fe065d Compare February 2, 2022 03:41
Most of the methods in CRM/Queue/Queue/Sql.php and CRM/Queue/Queue/SqlParallel.php are
identical. Move them to a common trait so that they're easier to patch.
@totten totten force-pushed the master-queue-cleanup branch from 9fe065d to 13812d8 Compare February 2, 2022 03:56
Consolidate duplicate implementations. Move to trait.
@totten
Copy link
Member Author

totten commented Feb 2, 2022

@artfulrobot @eileenmcnaughton - This should be a fairly straight-forward (REF).

@artfulrobot
Copy link
Contributor

@totten I've not r-run it, but on a brief visual inspection and with the assumption that the code has just been copy and pasted into the trait, looks good to me and the extra docblock comments are very valuable.

@eileenmcnaughton
Copy link
Contributor

Agree with @artfulrobot

I think it would be good to add type hints to some of these functions - but they are being moved unedited so out of scope

@eileenmcnaughton eileenmcnaughton merged commit 0733c79 into civicrm:master Feb 2, 2022
@totten totten deleted the master-queue-cleanup branch February 2, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants