Skip to content

Comments

fixed favicon method inconsistency with document and usages#5436

Merged
jxlwqq merged 3 commits intoz-song:masterfrom
ZsgsDesign:master
Mar 11, 2022
Merged

fixed favicon method inconsistency with document and usages#5436
jxlwqq merged 3 commits intoz-song:masterfrom
ZsgsDesign:master

Conversation

@ZsgsDesign
Copy link
Contributor

@ZsgsDesign ZsgsDesign commented Oct 11, 2021

In documents and views, favicon() was used like:

Admin::favicon('favicon.png');
@if(!is_null($favicon = Admin::favicon()))
    <link rel="shortcut icon" href="{{$favicon}}">
@endif

However, in Encore\Admin\Admin, it is inconsistently designated as public function rather than public static function. It doesn't affect actual usage right now but it is an inconsistency that should be fixed.

    /**
     * @param null|string $favicon
     *
     * @return string|void
     */
    public function favicon($favicon = null)
    {
        if (is_null($favicon)) {
            return static::$favicon;
        }

        static::$favicon = $favicon;
    }

@ZsgsDesign ZsgsDesign changed the title fixed favicon method consistency with document and usages fixed favicon method inconsistency with document and usages Oct 11, 2021
@jxlwqq
Copy link
Collaborator

jxlwqq commented Oct 11, 2021

Facades provide a "static" interface to classes that are available in the application's service container.

https://laravel.com/docs/8.x/facades

@ZsgsDesign ZsgsDesign closed this Oct 11, 2021
@ZsgsDesign ZsgsDesign reopened this Oct 11, 2021
@ZsgsDesign
Copy link
Contributor Author

ZsgsDesign commented Oct 11, 2021

@jxlwqq Still, when making declarations of methods, static methods should be consistent with usages, it is not a bug but about coding consistency. As stated earlier, it doesn't affect actual usage. But I still believe we should change that, as other static methods all declared with proper static.

@ZsgsDesign
Copy link
Contributor Author

Also, could you check another PR I opened earlier this morning at laravel-admin-extensions/scheduling#20

@jxlwqq
Copy link
Collaborator

jxlwqq commented Oct 11, 2021

use Encore\Admin\Facades\Admin;
Admin::favicon();

@ZsgsDesign
Copy link
Contributor Author

ZsgsDesign commented Oct 11, 2021

@jxlwqq As stated earlier, it doesn't affect actual usage. You can even call Encore\Admin\Admin::favicon() and it would be fine. It is just about code consistency, that other parts of Encore\Admin\Admin methods like js or style all uses static, and Laravel codes either. You can see Arr and Str all uses static declarations:

    /**
     * Cross join the given arrays, returning all possible permutations.
     *
     * @param  iterable  ...$arrays
     * @return array
     */
    public static function crossJoin(...$arrays)
    {
        $results = [[]];

        foreach ($arrays as $index => $array) {
            $append = [];

            foreach ($results as $product) {
                foreach ($array as $item) {
                    $product[$index] = $item;

                    $append[] = $product;
                }
            }

            $results = $append;
        }

        return $results;
    }

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 2, 2022
@stale stale bot removed the wontfix label Mar 11, 2022
@jxlwqq jxlwqq merged commit 2633381 into z-song:master Mar 11, 2022
@jxlwqq
Copy link
Collaborator

jxlwqq commented Mar 11, 2022

thanks

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