Skip to content

Enforced type of suffixes to array #301

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

Merged
merged 3 commits into from
Feb 12, 2019
Merged

Enforced type of suffixes to array #301

merged 3 commits into from
Feb 12, 2019

Conversation

xyNNN
Copy link
Contributor

@xyNNN xyNNN commented Feb 11, 2019

When the runtime including of the suffixes (self::$public_suffix_list = include __DIR__.'/../resources/public_suffix_list.php';) list don't work out the method return value is a boolean. So the in_array method will throw an error. Why this is implemented as a include instead of a private property or constant value?

@oscarotero
Copy link
Collaborator

Hi, thanks for this. I think this should be fixed in the getSuffixes() function. If you make this change, I'll merge the PR.

@xyNNN
Copy link
Contributor Author

xyNNN commented Feb 12, 2019

I've changed the implementation. When the including fails due to what ever reason the return value is a boolean, so we need so save it in a temporary variable and based on this value we have to decide.

@xyNNN
Copy link
Contributor Author

xyNNN commented Feb 12, 2019

It would be awesome if you could merge this and tag a new release :) thanks!

@oscarotero
Copy link
Collaborator

The problem is that you may want to do not use suffixes at all, and there's not a way to config this. I was thinking in something like this:

    private static function getSuffixes()
    {
        if (self::$public_suffix_list === null) {
            self::$public_suffix_list = (array) include __DIR__.'/../resources/public_suffix_list.php';
    	}
        return self::$public_suffix_list;
    }

@xyNNN
Copy link
Contributor Author

xyNNN commented Feb 12, 2019

You're right, I like your solution and have updated my PR.

@oscarotero oscarotero merged commit 78c6874 into php-embed:master Feb 12, 2019
@oscarotero
Copy link
Collaborator

Thanks!

@xyNNN
Copy link
Contributor Author

xyNNN commented Feb 12, 2019

Thanks @oscarotero - It's possible that you could tag a new release?

@oscarotero
Copy link
Collaborator

Done: https://github.com/oscarotero/Embed/releases/tag/v3.3.8

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.

2 participants