Fix #171: remove "illuminate/config" dependency

[klimov-paul]

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Tests pass?	✔️
Fixed issues	#171

[peterangelov]

Hi,

Removing dependency on the illuminate/config is good 👍

I wonder if it is really needed to pass the configuration array into the constructor? (see https://github.com/artesaos/seotools/pull/172/files#diff-f16dff94d6a0c6e9e422dfc0f99f0b66R122) This makes it hard to change config values for example in a unit test.

Why not reach for config values directly using for example config() helper method. Then it wouldn't be necessary to use Arr::get() like here https://github.com/artesaos/seotools/pull/172/files#diff-f16dff94d6a0c6e9e422dfc0f99f0b66R458

[klimov-paul]

I wonder if it is really needed to pass the configuration array into the constructor?

This is not my decision. It is an approach used throughout this entire library. See:

• https://github.com/artesaos/seotools/blob/master/src/SEOTools/OpenGraph.php#L156
• https://github.com/artesaos/seotools/blob/master/src/SEOTools/TwitterCards.php#L32
• and so on.

This makes it hard to change config values for example in a unit test.

I don't get it. You can easily setup any configuration for the object under test:

$objectUnderTest = new SEOMeta([/* test config goes here*/]);

or redefine corresponding service at the DI container:

app()->instance('seotools.metatags', new SEOMeta([/* custom config */]));

Why not reach for config values directly using for example config() helper method.

Using global helper function is not a good idea. See laravel/ideas#1569
Usage of config() helper will bind the object to the Laravel application global configuration, making it impossible to instantiate another object of the same class with different configuration.

[peterangelov]

Hi, thanks for your answer. For me it's not working, maybe I'm doing something wrong. Let me explain on the example.

I added a new config key to the file seometa.php to allow control for adding a class to the title tag. Now I want to add to the file https://github.com/artesaos/seotools/blob/master/tests/SEOTools/SEOMetaTest.php following method:

public function test_it_can_add_notranslate_class_to_title()
    {
        config(['seotools.meta.add_notranslate_class' => true]);
        
        $expected = "<title class=\"notranslate\">It's Over 9000!</title>";
        $expected .= "<meta name=\"description\" content=\"For those who helped create the Genki Dama\">";

        $this->setRightAssertion($expected);
    }

But the test fails, because I can not change the config value for seotools.meta.add_notranslate_class which is by default true (if I change the value in config to true, the test passes). Problem is that SEOMeta is a singleton in the app container with config values already in it, not being fetched from Laravel config. I tried your suggestion to redefine service in the test, but it didn't work.

[klimov-paul]

Your unit test should be following:

public function test_it_can_add_notranslate_class_to_title()
    {
        $seoMeta = new SEOMeta([
            'add_notranslate_class' => true,
            'defaults' => [
                'title' => 'meta title',
                'description' => 'meta description',
            ],
        ]);
        
        $expected = "<title class=\"notranslate\">meta title</title>";
        $expected .= "<meta name=\"description\" content=\"meta description\">";

        $this->assertSame($expected, $seoMeta->generate(true));
    }