WP_UnitTest_Factory_For_Attachment method for 'create' is not implemented

[bcole808]

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.

[boonebgorges]

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]

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]

@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?

[DrewAPicture]

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

[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?

[boonebgorges]

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().

[boonebgorges]

In 38330:

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

Legacy argument format continues to be accepted.

Props bcole808.
See #37630.

[boonebgorges]

In 38331:

Tests: Fix incorrect variable name from [38330].

See #37630.

[boonebgorges]

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 :)