WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37630 closed defect (bug) (fixed)

WP_UnitTest_Factory_For_Attachment method for 'create' is not implemented

Reported by: bcole808 Owned by: boonebgorges
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: docs Cc:

Description

With most of the unit test factories, it is possible to create test objects like this:

$this->factory->post->create();
$this->factory->user->create();
$this->factory->comment->create();

However, if you try to use the Attachment factory in the same way, it will cause an error like this:

$this->factory->attachment->create();

// strpos() expects parameter 1 to be string, array given
// ...../wp-includes/post.php:256

I found that the cause of this error is because the method signature in the Attachment factory to create objects differs from that of the other factories:

// WP_UnitTest_Factory_For_Thing
abstract function create_object( $args );

// WP_UnitTest_Factory_For_Attachment
function create_object( $file, $parent = 0, $args = array() )

And so the parameters end up being incorrect when using Create.

One possible solution could be to implement the Create method (and possibly others, such as create_and_get, create_many, etc) so that they accept the different parameters required by the Attachment factory. (I have attached a patch with a possible implementation of Create)

Another possible solution could be to re-factor the Attachment factory so it uses the same method signature as it's parent class, but that would be more complex as tests currently rely on it as-is.

Attachments (3)

37630.diff (1.3 KB) - added by bcole808 3 years ago.
37630.2.diff (959 bytes) - added by bcole808 3 years ago.
37630.3.diff (1.6 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (13)

@bcole808
3 years ago

#1 @ocean90
3 years ago

  • Keywords has-patch added
  • Version trunk deleted

#2 @boonebgorges
3 years ago

Another possible solution could be to re-factor the Attachment factory so it uses the same method signature as it's parent class, but that would be more complex as tests currently rely on it as-is.

This seems like the better strategy for internal consistency. However, we'll probably want to detect overloaded arguments for backward compatibility with third-party tools using the factory. So something like:

function create_object( $args, $legacy_parent = 0, $legacy_args = array() ) {
    if ( is_string( $args ) ) {
        $args = $legacy_args;
        $args['parent'] = $legacy_parent;
        $args['file'] = $args;
    }
}

or something like that. (We could also do variadic func_num_args() magic, but it's harder to read.)

What do you think?

@bcole808
3 years ago

#3 @bcole808
3 years ago

I agree that the better strategy would be to get the arguments to match the rest. Doing so would also fix methods like create_many() which currently don't work for the attachment factory.

Detecting old arguments would solve backwards compatibility, my only worry is that someone looking at the code might not understand why it is this way (for backwards compatibility).

New patch attached:
https://core.trac.wordpress.org/attachment/ticket/37630/37630.2.diff
with a possible implementation of this.

@boonebgorges
3 years ago

#4 @boonebgorges
3 years ago

  • Focuses docs added
  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 4.7

@bcole808 Thank you for the updated patch! The logic looks good to me. I have added some documentation to make it a bit clearer what's going on here.

@DrewAPicture Could we please have a documentation review of 37630.3.diff? Is the summary for $args clear enough (args get passed through to wp_insert_post())? Is the $legacy_ naming convention sufficient for deprecated params?

#5 @DrewAPicture
3 years ago

@boonebgorges I think that's clear enough. +1

#6 follow-up: @bcole808
3 years ago

The params for filename and post parent ID are both optional for wp_insert_attachment, shouldn't we keep them optional for the factory as well instead of returning a WP Error in case they are missing?

#7 in reply to: ↑ 6 @boonebgorges
3 years ago

  • Keywords dev-feedback removed
  • Owner set to boonebgorges
  • Status changed from new to assigned

Replying to bcole808:

The params for filename and post parent ID are both optional for wp_insert_attachment, shouldn't we keep them optional for the factory as well instead of returning a WP Error in case they are missing?

I believe that these params are optional only because it's possible to pass them in the $args array instead. If you don't provide a file and a parent, the post you create won't be a valid attachment, at least not for the purpose of doing unit testing. But I suppose there's no reason this has to be enforced by the factory - we can leave it up to people writing tests, if it's more consistent with wp_insert_attachment().

#8 @boonebgorges
3 years ago

In 38330:

Tests: Attachment create() method should match signature of other create() methods.

Legacy argument format continues to be accepted.

Props bcole808.
See #37630.

#9 @boonebgorges
3 years ago

In 38331:

Tests: Fix incorrect variable name from [38330].

See #37630.

#10 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

I was going to go through and change all existing instances of create_object() to create() with the new syntax, but there are many dozens, and they must be done manually. If anyone wants easy props, they can submit a patch :)

Note: See TracTickets for help on using tickets.