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

Move edac_insert_rule_data to an inserter class #513

Conversation

pattonwebz
Copy link
Member

@pattonwebz pattonwebz commented Feb 23, 2024

Moves the edac_insert_rule_data function to a new Insert_Rule_Data class with a single method named insert that takes the same passed params.

  • I added type hints on the parameters (should post actually be a \WP_Post type rather than an object?).
  • Added an early bail if one of the required params aren't passed (typehints technically prevent this happening but having it makes it more clear what is going on).
  • Started adding tests (which still needs more work to cover more of the method).

I have some thoughts about potential refactor of this function so that it can have return values that are likely to give more certainty about what has happened. My initial thought was bool|int and never void. With true being success, false being fail and the int meaning it was a duplicate but would also evaluate to a truth value for checking that might be added later after it's called.

Closes: #464

@pattonwebz pattonwebz marked this pull request as draft February 23, 2024 23:50
@SteveJonesDev
Copy link
Member

@pattonwebz, are you still working on this PR since it's in draft mode?

@pattonwebz
Copy link
Member Author

@pattonwebz, are you still working on this PR since it's in draft mode?

I still need to work on the tests here for better coverage. If you want to review the actual code changes, that'd be great.

I plan to revisit the tests here tomorrow and the other issues assigned to me.

@pattonwebz pattonwebz marked this pull request as ready for review February 28, 2024 20:02
Don't really need to wait for any reason here
admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
includes/deprecated/deprecated.php Outdated Show resolved Hide resolved
includes/deprecated/deprecated.php Outdated Show resolved Hide resolved
if ( ! $results ) {

// filter post types.
$rule_data = apply_filters( 'edac_filter_insert_rule_data', $rule_data );
Copy link
Member

@SteveJonesDev SteveJonesDev Mar 12, 2024

Choose a reason for hiding this comment

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

@pattonwebz, should we sanitize this data to ensure it's been filtered in a safe manner?

Example:

// Apply filters to allow modification of the rule data.
$rule_data = apply_filters( 'edac_filter_insert_rule_data', $rule_data );

// Sanitize the filtered data.
$rule_data_sanitized = array(
    'postid'        => absint( $rule_data['postid'] ),
    'siteid'        => absint( $rule_data['siteid'] ),
    'type'          => sanitize_text_field( $rule_data['type'] ),
    'rule'          => sanitize_text_field( $rule_data['rule'] ),
    'ruletype'      => sanitize_text_field( $rule_data['ruletype'] ),
    'object'        => sanitize_text_field( $rule_data['object'] ),
    'recordcheck'   => absint( $rule_data['recordcheck'] ),
    'user'          => absint( $rule_data['user'] ),
    'ignre'         => absint( $rule_data['ignre'] ),
    'ignore_user'   => isset( $rule_data['ignore_user'] ) ? absint( $rule_data['ignore_user'] ) : null,
    'ignore_date'   => isset( $rule_data['ignore_date'] ) ? sanitize_text_field( $rule_data['ignore_date'] ) : null,
    'ignore_comment'=> isset( $rule_data['ignore_comment'] ) ? sanitize_text_field( $rule_data['ignore_comment'] ) : null,
    'ignore_global' => absint( $rule_data['ignore_global'] ),
);

// Now, $rule_data_sanitized contains the sanitized data ready for insertion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that you fixed the ignre to ignore, but we can't do that without first remapping the columns in the table. Would you like me to make an issue to tackle that in the future?

admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
admin/class-insert-rule-data.php Outdated Show resolved Hide resolved
* @return void|int
*/
function edac_insert_rule_data( $post, $rule, $ruletype, $rule_obj ) {
_deprecated_function( __FUNCTION__, '2.0.0', 'EDAC\Admin\Insert_Rule_Data' );
Copy link
Member

Choose a reason for hiding this comment

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

Update version to 1.10.0

Copy link
Member

@SteveJonesDev SteveJonesDev left a comment

Choose a reason for hiding this comment

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

Good to merge.

@SteveJonesDev SteveJonesDev added this to the v1.10.0 milestone Mar 18, 2024
@pattonwebz pattonwebz merged commit 8b3485e into equalizedigital:develop Mar 18, 2024
11 checks passed
@pattonwebz pattonwebz deleted the william/464/move-edac_insert_rule_data-to-a-class branch March 18, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert edac_insert_rule_data function to a class
2 participants