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

ExtensionsExtension: Fix type hint of RC #209

Merged
merged 2 commits into from Jul 19, 2019

Conversation

janbarasek
Copy link
Contributor

@janbarasek janbarasek commented Jul 18, 2019

  • bug fix
  • BC break? no

Fix type hint and solve error:

------ --------------------------------------------------------------------- 
  Line   DI/Extensions/ExtensionsExtension.php                                
 ------ --------------------------------------------------------------------- 
  34     Parameter #2 $extension of method Nette\DI\Compiler::addExtension()  
         expects Nette\DI\CompilerExtension, object given.                    
 ------ --------------------------------------------------------------------- 

@dg dg force-pushed the master branch 2 times, most recently from edf67d2 to b365d4c Compare Jul 19, 2019
@@ -30,8 +30,9 @@ public function loadConfiguration()
$name = null;
}
if ($class instanceof Nette\DI\Definitions\Statement) {
$rc = new \ReflectionClass($class->getEntity());
$this->compiler->addExtension($name, $rc->newInstanceArgs($class->arguments));
/** @var Nette\DI\CompilerExtension $rcExtension */
Copy link
Member

@dg dg Jul 19, 2019

Choose a reason for hiding this comment

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

In fact, we don't know if the $rcExtension is CompilerExtension, and it is expected that it might throw a TypeError.

Copy link
Contributor

@mabar mabar Jul 19, 2019

Choose a reason for hiding this comment

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

Btw, it is better to use assert($rcExtension instanceof CompilerExtension)
Locally defined annotations are hack to satisfy IDE

Copy link
Member

@dg dg Jul 19, 2019

Choose a reason for hiding this comment

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

Ideally, there should be checking and throwing an exception.

Copy link
Contributor

@mabar mabar Jul 19, 2019

Choose a reason for hiding this comment

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

assert should be used instead of local annotations for internal behavior, to be exact.

Copy link
Contributor Author

@janbarasek janbarasek Jul 19, 2019

Choose a reason for hiding this comment

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

I implemented exception, but I'm not sure about the message.

@dg dg force-pushed the master branch 3 times, most recently from 89b7ae6 to c60c7ff Compare Jul 19, 2019
@dg dg force-pushed the master branch 2 times, most recently from fecd1fb to 15978c3 Compare Jul 19, 2019
@dg dg merged commit 061f042 into nette:master Jul 19, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants