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

Docs do not match functionality #6141

Closed
wants to merge 2 commits into from
Closed

Docs do not match functionality #6141

wants to merge 2 commits into from

Conversation

Loupax
Copy link
Contributor

@Loupax Loupax commented Jan 13, 2016

Q A
Doc fix? yes
New docs? no
Applies to 3
Fixed tickets None

The configuration section specifies that one of the requirements that need to be met to automatically dump configuration is the constructor not requiring parameters.

The code that handles this though does not match this description:

/**
 * {@inheritdoc}
 */
public function getConfiguration(array $config, ContainerBuilder $container)
{
    $reflected = new \ReflectionClass($this);
    $namespace = $reflected->getNamespaceName();

    $class = $namespace.'\\Configuration';
    if (class_exists($class)) {
        $r = new \ReflectionClass($class);
        $container->addResource(new FileResource($r->getFileName()));

        // Notice here that it just checks if the __construct method exists,
        // the arguments are irrelevant
        if (!method_exists($class, '__construct')) {
            $configuration = new $class();

            return $configuration;
        }
    }
}

@psampaz
Copy link

psampaz commented Jan 13, 2016

👍

(``YourBundle\DependencyInjection\Configuration``) and does not require
arguments to be passed to the constructor it will work automatically. If you
(``YourBundle\DependencyInjection\Configuration``) and does not extend
its constructor it will work automatically. If you
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"extend" sounds strange when we talk about methods imo. What about saying something like "and does have a constructor"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works either way, I just used extend because this still works if the parent Configuration class has a __construct method itself.

Still I agree that "does not have a constructor" makes the meaning very clear. Waiting for any other feedback before changing the PR :)

@psampaz
Copy link

psampaz commented Jan 18, 2016

👍 for "does not have a constructor"

@Loupax
Copy link
Contributor Author

Loupax commented Jan 28, 2016

Cannot understand why the CI test failed. Is there a way to re-run it without re-commiting?

@wouterj
Copy link
Member

wouterj commented Jan 28, 2016

@Loupax don't worry about the CI test. That's an issue that is not related to this PR (and in fact, already fixed on the 2.3 branch).

👍 from me for merging this PR. Thanks for fixing the docs!

@xabbuh
Copy link
Member

xabbuh commented Jan 29, 2016

Thank you @Loupax.

xabbuh added a commit that referenced this pull request Jan 29, 2016
This PR was submitted for the 3.0 branch but it was merged into the 2.3 branch instead (closes #6141).

Discussion
----------

Docs do not match functionality

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 3
| Fixed tickets | None

The Dump the configuration section specifies that one of the requirements that need to be met to automatically dump configuration is the constructor not requiring parameters.

The code that handles this though does not match this description:

```
/**
 * {@inheritdoc}
 */
public function getConfiguration(array $config, ContainerBuilder $container)
{
    $reflected = new \ReflectionClass($this);
    $namespace = $reflected->getNamespaceName();

    $class = $namespace.'\\Configuration';
    if (class_exists($class)) {
        $r = new \ReflectionClass($class);
        $container->addResource(new FileResource($r->getFileName()));

        // Notice here that it just checks if the __construct method exists,
        // the arguments are irrelevant
        if (!method_exists($class, '__construct')) {
            $configuration = new $class();

            return $configuration;
        }
    }
}
```

Commits
-------

739e23a Docs do not match functionality
@xabbuh xabbuh closed this Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants