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

Add more utilities to stub WordPress functions #125

Open
antonioeatgoat opened this issue Jul 29, 2022 · 3 comments
Open

Add more utilities to stub WordPress functions #125

antonioeatgoat opened this issue Jul 29, 2022 · 3 comments

Comments

@antonioeatgoat
Copy link

antonioeatgoat commented Jul 29, 2022

Right now, BrainMonkey already covers all those functions that you have to stubs in 90% of the cases, when you write enough unit tests for your code.

However, there are many functions that are not used so often, but still they are used quite enough and same enough to avoid duplication in every unit test setup.

Here's a link of all the functions I've spotted.

home_url()

Also if this function is very simple and in most of the case the stub only returns a static string, it is used really a lot and in many cases it must be stubbed with a callback usage.

sanitize_text_field()

This is used quite often as well, however I see that sometimes it is aliased with a callback very specific to a single test, so maybe it needs to be called in a separate function?

wp_validate_boolean()

Also if this is one of the less stubbed, I believe we can assume it always will need the same alias, so probably it worth to be centralized as well.

add_query_arg()

This isn't very used neither, but it's very boring to stub if you need to alias it.

wp_slash()

Again, not used so often but very simple to centralize in my opinion.

How to proceed

I think that one of the important things to decide, in addition to specific aliases, is how to "group" the stubs. Right now, the package provides the functions stubTranslationFunctions() and stubEscapeFunctions() and we can assume that if I need one of the functions stubbed there, then I need also the others. However, about the functions named above, I could want a generic alias for certain functions but some specific alias created by me for others, and I am not sure that the package allows to "override" the alias in some way.

@gmazzap
Copy link
Collaborator

gmazzap commented Jul 29, 2022

Thank you @antonioeatgoat !

I am not sure that the package allows to "override" the alias in some way.

Yes, you can always override stubs.


Now about functions:

home_url()

The problem is: what do you return from the stub? I don't think we can rely on an hardcoded URL.

Maybe we can have a stubUrlsByConstants() which, if constants like SITE_URL and HOME_URL are defined, would stub functions like home_url(), get_home_url(), site_url(), admin_url(), and possibly others.
So in theory you could define those two constants in the test bootstrap, call stubUrlsByConstants() in the base test case, and enjoy 7/8 functions stubbed for you.


sanitize_text_field()

Not a fun of messing with this. Let's assume we stub with a simplified version, and that simplified version make the test pass. That will give developers confidence in their code. But then in WP context is fails. Considering we're talking of security-related function, I want to avoid that.

I agree many times in tests you don't really rely on sanitization, but for these cases we have stubs() function. In the base test case you can have:

Monkey\Funcions\stubs(
    [
        'sanitize_text_field',
        /* possibly others... */
    ]
);

it requires a minimum effort, to be done once per package, and you have a minimum viable behavior: those stubs would return back what you pass to them, and you decide for your package what to stub.


wp_validate_boolean()

To be honest, I pretty much never use this because filter_var($var, FILTER_VALIDATE_BOOLEAN) does it better. If I use wp_validate_boolean is because I want to be able to "force" it to return a given value in tests. But yeah, I see value in having the function pre-declared as one-line "helper"

if ( ! function_exists('wp_validate_boolean')) {
    function wp_validate_boolean($var)
    {
        return is_string($var) && (strtolower($var) === 'false') ? false : (bool)$var;
    }
}

add_query_arg

The logic of this funcions is very complex. And the reason is that it accepts different type of arguments.
You cal call it like:

add_query_arg('key', $value); // add the "key" query var with $value to current URL
add_query_arg('key', false); // remove the "key" query var from current URL
add_query_arg('key', $value, $url); // add the "key" query var with $value to given URL
add_query_arg('key', false, $url);    // remove the "key" query var from given URL
add_query_arg(['key' => $value, 'key2' => false]);  // add the "key" query var with $value and remove 'key2' from current URL
add_query_arg(['key' => $value, 'key2' => false], $url);  // add the "key" query var with $value and remove 'key2' from given URL

And it takes into account if the current/given URL has already vars, plus does URL-encoding.

This complexity does not allow to "simplify" the function. Any simplification would cause a difference in behavior compared to WP, with hight risk of false positives or false negatives.

So the only whay to stub this function in a generic way is to copy the code from WP... which is something I don't to do.

In specific tests, you know how the function is used. So you don't need to stub all possible usages, but just the usage your specific code has in that specific method you're testing. So that is easier to stub.

Code that is heavily relying on add_query_arg and requires it to behave exactly how it does on WP to be tested properly... is probably better tested in integration.


wp_slash()

Maybe another candidate for an helper.

function wp_slash($value) {
    if (is_array($value)) {
        return array_map('wp_slash', $value);
    }
    return is_string($value) ? addslashes($value) : $value;
}

not a one-liner, but small enough.


In summary

  • I am in favor of a stubUrlsByConstants() which stubs multiple URL-related functions if URL-related constants are defined. Maybe we could consider looking into env variables, to make it easier to change values across tests.
  • I am in favor of having pre-defined helpers for wp_validate_boolean and wp_slash.
  • I would leave the other functions alone.

@antonioeatgoat
Copy link
Author

antonioeatgoat commented Aug 1, 2022

Thank you @gmazzap for your feedback! We are quite in line, all the ones you discarded are the ones I had most doubts about.

About home_url() instead. my idea was something like this:

function stubUrls(string $domain) { // Definitely needed to find a better name

  \Monkey\Functions\when('home_url')->alias(
      function (string $path = '') use ($domain): string {
          return $domain . '/' . ltrim($path, '/');
      }
  );

  // ...
  // And so for the other functions to stub..
  // ...
}

@gmazzap
Copy link
Collaborator

gmazzap commented Aug 2, 2022

Indeed, something like this should be fine (please note this repo still supports PHP 5.6):

function stubWpUrls($domain, $useHttps = true) {

    $schema = $useHttps ? 'https://' : 'http://';

    $url = static function ($suffix = '') use ($domain, $schema) {
        return static function ($path = '') use ($suffix , $domain, $schema) {
            return $schema . $domain . suffix . ltrim($path, '/');
        };
    };

    $urlBySite = static function ($suffix = '') use ($domain, $schema) {
         return static function ($siteId, $path = '') use ($suffix , $domain, $schema) {
            return $schema . $domain . $suffix  . ltrim($path, '/');
         };
    };
  
    \Monkey\Functions\stubs([
        'home_url' => $url('/'),
        'get_home_url' => $urlBySite ('/'), 
        'site_url' => $url('/'),
        'get_site_url' => $urlBySite('/'),
        'admin_url' => $url('wp-admin/'),
        'get_admin_url' => $urlBySite('wp-admin/'),
        'content_url' => $url('wp-content/'),
        'rest_url' => $url('wp-json/'),
        'get_rest_url' => $urlBySite('wp-json/'),
        'includes_url' => $url('wp-includes/'),
        'network_home_url' => $url('/'),
        'network_site_url' => $url('/'),
        'network_admin_url' => $url('wp-admin/network/'),
        'user_admin_url' => $url('wp-admin/user/'),
    ]);
}

gmazzap added a commit that referenced this issue Dec 12, 2022
It makes use of a new UrlsHelper class.

Tests and documentation added.

See #125
gmazzap added a commit that referenced this issue Dec 12, 2022
gmazzap added a commit that referenced this issue Dec 12, 2022
paulshryock added a commit to paulshryock/BrainMonkey that referenced this issue Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants