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

Fix parsing invalid href #502

Merged
merged 4 commits into from
Oct 10, 2022

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?

src/Detectors/Languages.php Outdated Show resolved Hide resolved
src/Detectors/Languages.php Outdated Show resolved Hide resolved
@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
Owner

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 oscarotero:master Oct 10, 2022
@oscarotero
Copy link
Owner

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