Skip to content

Conversation

@grandeljay
Copy link
Contributor

This is my attempt to fix #501

In this case, I'd create a helper, so it can be used in other places. Something like:

@oscarotero I wasn't sure what exactly you mean, but maybe something like this?

@grandeljay
Copy link
Contributor Author

Sorry you had to see that. This time I tested my changes.

I'm also wondering if it would make sense to use the splat operator so we could use

if (self::isEmpty($language, $href)) {

instead?

private function isEmpty(string ...$values): bool
{
    $skipValues = array(
        'undefined',
    );

    foreach ($values as $value) {
        if (empty($value) || in_array($value, $skipValues)) {
            return true;
        }
    }

    return false;
}

@oscarotero
Copy link
Collaborator

I'm also wondering if it would make sense to use the splat operator

Ok, but the arguments can be of any type, not only string (it must return true for null, 0, [], etc).

The isEmpty function shouldn't be a private method but a function that can be imported by other classes. The global helpers are located in this file: https://github.com/oscarotero/Embed/blob/master/src/functions.php

@grandeljay
Copy link
Contributor Author

grandeljay commented Oct 10, 2022

Ok, but the arguments can be of any type, not only string (it must return true for null, 0, [], etc).

getAttribute() seems to return a nullable string. If this helper is just used for this method, I think requiring a string parameter makes sense.

If you want to make the helper more "future proof" by allowing all types, I don't think we can continue to use empty() since it

Returns true if var does not exist or has a value that is empty or equal to zero, aka falsey [...]

So null, 0 and [] would be considered empty values which you do not seem to want.

The isEmpty function shouldn't be a private method but a function that can be imported by other classes. The global helpers are located in this file: https://github.com/oscarotero/Embed/blob/master/src/functions.php

Thanks! I'll change it!

Additionally, the function was refactored to make use of the splat operator
Since it is not very clear how isEmpty() works, I added a short note to clarify that it returns true if one of the supplied variables is empty (instead of all of them).
@oscarotero oscarotero merged commit 17b892c into php-embed:master Oct 10, 2022
@oscarotero
Copy link
Collaborator

Looks good to me. Thanks!

grandeljay added a commit to wishthis/wishthis that referenced this pull request Oct 10, 2022
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.

The path of a URI with an authority must start with a slash "/" or be empty

2 participants