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.

Sign up for GitHub

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

Refactor: Remove side effects from data provider #7478

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
+36 −32
Open

Refactor: Remove side effects from data provider #7478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05e24d3
Refactor: Remove side effects from data provider
Aug 5, 2024
6784617
fix: Corrects expected values after removing edge effect
Sep 5, 2024
292da87
fix: avoid instantiating the class multiple times
Sep 5, 2024
031772e
fix: Explain `ENGINE=InnoDB` to be able to use `TRANSACTION` in tests
Sep 5, 2024
30a6faf
fix: misprint
Sep 5, 2024
08d152f
Merge branch 'develop' into fix/test/test-misc-functions
jonwaldstein Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion includes/database/class-give-db-comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public function create_table() {
comment_date datetime NOT NULL,
comment_date_gmt datetime NOT NULL,
PRIMARY KEY (comment_ID)
) {$charset_collate};";
) {$charset_collate} ENGINE=InnoDB;";

require_once ABSPATH . 'wp-admin/includes/upgrade.php';
dbDelta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion includes/database/class-give-db-donors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ public function create_table() {
PRIMARY KEY (id),
UNIQUE KEY email (email),
KEY user (user_id)
) CHARACTER SET utf8 COLLATE utf8_general_ci;";
) CHARACTER SET utf8 COLLATE utf8_general_ci ENGINE=InnoDB;";

dbDelta( $sql );

Expand Down
4 changes: 2 additions & 2 deletions includes/database/class-give-db-meta.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ public function __call( $name, $arguments ) {
$id = $arguments[1];
$meta_key = $arguments[2];
$meta_value = $arguments[3];
$delete_all = $arguments[3];
$delete_all = $arguments[4];
$this->is_filter_callback = true;

// Bailout.
Expand Down Expand Up @@ -543,7 +543,7 @@ public function create_table() {
PRIMARY KEY (meta_id),
KEY {$this->meta_type}_id ({$this->meta_type}_id),
KEY meta_key (meta_key({$this->min_index_length}))
) {$charset_collate};";
) {$charset_collate} ENGINE=InnoDB;";

require_once ABSPATH . 'wp-admin/includes/upgrade.php';
dbDelta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion includes/database/class-give-db-sequential-ordering.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public function create_table() {
id bigint(20) NOT NULL AUTO_INCREMENT,
payment_id bigint(20) NOT NULL,
PRIMARY KEY (id)
) {$charset_collate};";
) {$charset_collate} ENGINE=InnoDB;";

require_once ABSPATH . 'wp-admin/includes/upgrade.php';
dbDelta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion includes/database/class-give-db-sessions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function create_table() {
session_expiry BIGINT UNSIGNED NOT NULL,
PRIMARY KEY (session_key),
UNIQUE KEY session_id (session_id)
) {$charset_collate};";
) {$charset_collate} ENGINE=InnoDB;";

require_once ABSPATH . 'wp-admin/includes/upgrade.php';
dbDelta( $sql );
Expand Down
16 changes: 8 additions & 8 deletions includes/install.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,14 +515,14 @@ function give_install_tables_on_plugin_update($old_version)
function __give_get_tables()
{
$tables = [
'donors_db' => new Give_DB_Donors(),
'donor_meta_db' => new Give_DB_Donor_Meta(),
'comment_db' => new Give_DB_Comments(),
'comment_db_meta' => new Give_DB_Comment_Meta(),
'give_session' => new Give_DB_Sessions(),
'formmeta_db' => new Give_DB_Form_Meta(),
'sequential_db' => new Give_DB_Sequential_Ordering(),
'donation_meta' => new Give_DB_Payment_Meta(),
'donors_db' => give()->get(Give_DB_Donors::class),
'donor_meta_db' => give()->get(Give_DB_Donor_Meta::class),
'comment_db' => give()->get(Give_DB_Comments::class),
'comment_db_meta' => give()->get(Give_DB_Comment_Meta::class),
'give_session' => give()->get(Give_DB_Sessions::class),
'formmeta_db' => give()->get(Give_DB_Form_Meta::class),
'sequential_db' => give()->get(Give_DB_Sequential_Ordering::class),
'donation_meta' => give()->get(Give_DB_Payment_Meta::class),
Comment on lines +518 to +525
Copy link
Contributor

Choose a reason for hiding this comment

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

What's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these classes are extensions of Give_DB_Meta.

However, this class, during its instantiation, will override all post modification methods via hooks such as:

if (in_array('delete_post_metadata', $this->supports)) {
    add_filter('delete_post_metadata', [$this, '__delete_meta'], 0, 5);
}

This will trigger a bugged behavior if one of these classes is instantiated more than once.

In the specific case of deletion, the return value indicates the number of rows deleted from the database.

But if the hook is called by several instances of the class, this is what will happen:

  1. The first hook will do the deletion work and return the number of rows deleted
  2. The second hook will look for lines to delete, find none (they were all deleted by the first hook) and therefore return the value 0
  3. The hook ends by systematically returning 0, the return value is wrong.

For this reason, I made the minimum possible modifications to approach singleton pattern behavior.

This is the code you present here.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 interesting!

I think the singleton pattern here does make sense 👍

We'll just have to run it through QA internally to make sure everything works as expected.

];

return $tables;
Expand Down
2 changes: 1 addition & 1 deletion src/EventTickets/Migrations/CreateEventTicketTypesTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function run() {
created_at DATETIME NOT NULL,
updated_at DATETIME NOT NULL,
PRIMARY KEY (id)
) $charset";
) $charset ENGINE=InnoDB;";
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my machine where the default engine is MyISAM, many of the tests crash because of their interactions.

Indeed, the teardown cleanup function of legacy tests uses SQL transactions with ROLLBACK to undo any modification of the DB by the test.

But MyISAM does not manage transactions...

This is why I suggest explicit use of the InnoDB engine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pulsovi thanks for the clarification


try {
DB::delta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion src/EventTickets/Migrations/CreateEventTicketsTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function run() {
created_at DATETIME NOT NULL,
updated_at DATETIME NOT NULL,
PRIMARY KEY (id)
) $charset";
) $charset ENGINE=InnoDB;";

try {
DB::delta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion src/EventTickets/Migrations/CreateEventsTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function run() {
created_at DATETIME NOT NULL,
updated_at DATETIME NOT NULL,
PRIMARY KEY (id)
) $charset";
) $charset ENGINE=InnoDB;";

try {
DB::delta( $sql );
Expand Down
2 changes: 1 addition & 1 deletion src/LegacySubscriptions/includes/give-subscriptions-db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ public function create_table()
KEY customer (customer_id),
KEY transaction (transaction_id),
INDEX customer_and_status ( customer_id, status)
) CHARACTER SET utf8 COLLATE utf8_general_ci;';
) CHARACTER SET utf8 COLLATE utf8_general_ci ENGINE=InnoDB;';

dbDelta($sql);

Expand Down
2 changes: 1 addition & 1 deletion src/Log/Migrations/CreateNewLogTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function run()
KEY log_type (log_type),
KEY category (category),
KEY source (source)
) {$charset}";
) {$charset} ENGINE=InnoDB;";

try {
DB::delta($sql);
Expand Down
2 changes: 1 addition & 1 deletion src/MigrationLog/Migrations/CreateMigrationsTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function run()
error text NULL,
last_run DATETIME NOT NULL,
PRIMARY KEY (id)
) {$charset}";
) {$charset} ENGINE=InnoDB;";

try {
DB::delta($sql);
Expand Down
2 changes: 1 addition & 1 deletion src/Revenue/Migrations/CreateRevenueTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function run()
form_id bigint UNSIGNED NOT NULL,
amount int UNSIGNED NOT NULL,
PRIMARY KEY (id)
) {$charset_collate};";
) {$charset_collate} ENGINE=InnoDB;";

try {
DB::delta($sql);
Expand Down
2 changes: 2 additions & 0 deletions src/ServiceProviders/LegacyServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,10 @@ private function bindInstance($alias, $class, $includesPath, $singleton = false)
give()->instance($alias, $class());
} elseif ($singleton) {
give()->instance($alias, $class::get_instance());
give()->alias($alias, $class);
} else {
give()->instance($alias, new $class());
give()->alias($alias, $class);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Subscriptions/Migrations/CreateSubscriptionTables.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public function run()
KEY `customer` (`customer_id`),
KEY `transaction` (`transaction_id`),
KEY `customer_and_status` (`customer_id`,`status`)
) $charset_collate;
) $charset_collate ENGINE=InnoDB;
"
);

Expand All @@ -61,7 +61,7 @@ public function run()
PRIMARY KEY (`meta_id`),
KEY `subscription_id` (`subscription_id`),
KEY `meta_key` (`meta_key`(191))
) $charset_collate;
) $charset_collate ENGINE=InnoDB;
"
);
} catch (DatabaseQueryException $exception) {
Expand Down
4 changes: 2 additions & 2 deletions tests/includes/legacy/tests-donors-db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ public function test_get_donors() {

$donors = Give()->donors->get_donors();

$this->assertEquals( 2, count( $donors ) );
$this->assertEquals( 1, count( $donors ) );

}

Expand All @@ -258,7 +258,7 @@ public function test_get_donors() {
*/
public function test_count_customers() {

$this->assertEquals( 2, intval( Give()->donors->count() ) );
$this->assertEquals( 1, intval( Give()->donors->count() ) );

$args = array(
'date' => array(
Expand Down
2 changes: 1 addition & 1 deletion tests/includes/legacy/tests-donors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ public function test_get_donor_address() {
*/
public function test_count_total_donors() {
$donor_count = give_count_total_donors();
$this->assertEquals( 2, $donor_count );
$this->assertEquals( 1, $donor_count );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/includes/legacy/tests-email-tags.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ function test_give_email_tag_metadata() {
Give()->donor_meta->update_meta( $donor_id, '_give_stripe_customer_id', 2 );

$this->assertEquals( 2, __give_render_metadata_email_tag( '{meta_donor__give_stripe_customer_id}', $donor_tag_args ) );
$this->assertEquals( 1, __give_render_metadata_email_tag( '{meta_donor_id}', $donor_tag_args ) );
$this->assertEquals( $donor_id, __give_render_metadata_email_tag( '{meta_donor_id}', $donor_tag_args ) );
$this->assertEquals( 1, __give_render_metadata_email_tag( '{meta_donor_user_id}', $donor_tag_args ) );
$this->assertEquals( 'Admin User', __give_render_metadata_email_tag( '{meta_donor_name}', $donor_tag_args ) );
$this->assertEquals( '[email protected]', __give_render_metadata_email_tag( '{meta_donor_email}', $donor_tag_args ) );
Expand Down
12 changes: 7 additions & 5 deletions tests/includes/legacy/tests-misc-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ public function give_get_currency_name_data_provider() {
*
* @dataProvider give_meta_helpers_provider
*/
public function test_give_meta_helpers( $form_or_donation_id ) {
public function test_give_meta_helpers( $form_or_donation_id_generator ) {
$form_or_donation_id = $form_or_donation_id_generator();

$value = give_get_meta( $form_or_donation_id, 'testing_meta', true, 'TEST1' );
$this->assertEquals( 'TEST1', $value );

Expand Down Expand Up @@ -129,10 +131,10 @@ public function test_give_meta_helpers( $form_or_donation_id ) {
* @access private
*/
public function give_meta_helpers_provider() {
return array(
array( Give_Helper_Payment::create_simple_payment() ),
array( Give_Helper_Form::create_simple_form()->id ),
);
return [
[function () { return Give_Helper_Payment::create_simple_payment(); }],
[function () { return Give_Helper_Form::create_simple_form()->id; }],
];
}

/**
Expand Down
Loading