Skip to content

Conversation

@JhonnyL
Copy link
Contributor

@JhonnyL JhonnyL commented Apr 27, 2016

Q A
Branch? 2.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Add check to make sure $bundle is an instance of BundleInterface. Prevents undefined method fatal error. e.g. getName().

@stof
Copy link
Member

stof commented Apr 27, 2016

-1 for that. We don't put safeguards everywhere in Symfony for people implementing methods without respecting the return type. Adding it in a single place would be inconsistent (and adding it everywhere would be a huge pain to maintain)

@JhonnyL
Copy link
Contributor Author

JhonnyL commented Apr 27, 2016

It could also be considered a corner case. Checks like this is often not needed for methods similar to initializeBundles since those methods often uses adderMethods with typehints that works as safeguards. Since initializeBundles does not use a initializeBundle method I think we should enforce the type in another way.

@stof
Copy link
Member

stof commented Apr 27, 2016

It could also be considered a corner case. Checks like this is often not needed for methods similar to initializeBundles since those methods often uses adderMethods with typehints that works as safeguards.

Check in Symfony (and related libraries maintained by us, for instance twig) for the usage of any method returning something, and you will see that often is not justified in your sentence.

@nicolas-grekas
Copy link
Member

Closing for the reasons given by @stof

@ro0NL
Copy link
Contributor

ro0NL commented Apr 28, 2016

We don't put safeguards everywhere in Symfony for people implementing methods without respecting the return type

@stof check #18656, it's done for same reasoning right? So we have a safeguard here.. I guess it's not a big deal but perhaps it could require some convention (i.e. allowed to improve logical exception messages)...

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.

6 participants