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.

Sign up for GitHub

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

Jump to bottom

Make IContainer et all typed #11595

Closed
wants to merge 3 commits into from
Closed

Make IContainer et all typed #11595

wants to merge 3 commits into from

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 3, 2018

@MorrisJobke found this branch of mine lying around.

Lets see what CI thinks

@rullzer
Make IContainer et all typed
ce2ef2a 
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer added this to the Nextcloud 15 milestone Oct 3, 2018
ChristophWurst
ChristophWurst approved these changes Oct 3, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good

MorrisJobke
MorrisJobke reviewed Oct 3, 2018
View reviewed changes
lib/private/AppFramework/Utility/SimpleContainer.php Outdated
@@ -90,7 +91,7 @@ private function buildClass(ReflectionClass $class) {
* @return \stdClass
* @throws QueryException if the class could not be found or instantiated
*/
public function resolve($name) {
public function resolve(string $name): \stdClass {
Copy link
Member

Choose a reason for hiding this comment

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

💥 💥 💥
TypeError: Return value of OC\AppFramework\Utility\SimpleContainer::resolve() must be an instance of stdClass, instance of OC\Support\CrashReport\Registry returned in

@ChristophWurst
Copy link
Member 

1) Test\AppFramework\DependencyInjection\DIIntergrationTests::testInjectFromServer
TypeError: Return value of OC\AppFramework\DependencyInjection\DIContainer::getServer() must implement interface OCP\IServerContainer, instance of OC\ServerContainer returned

I love this class hierarchy.

\OC\ServerContanier extends \OC\SimpleContainer
\OC\SimpleContanier implements \OCP\IContainer
\OCP\IServerContainer extends \OCP\IContainer
\OC\Server extends OC\ServerContainer and implments \IServerContainer

so DIContainer::getServer should either just return IContainer or we have to change the class/interface hierarchy.

@rullzer
Copy link
Member Author

rullzer commented Oct 10, 2018

Yeah it is messy... 🙊

I think this test is failing because we do sneaky things in the test. I'll check it out

@MorrisJobke MorrisJobke mentioned this pull request Nov 7, 2018
Merged
29 tasks
@MorrisJobke
Copy link
Member

@rullzer I guess nothing for 15, right?

@rullzer rullzer removed this from the Nextcloud 15 milestone Nov 7, 2018
@rullzer
Copy link
Member Author

rullzer commented Nov 7, 2018

Correct

@rullzer
Copy link
Member Author

rullzer commented Jan 14, 2019

Lets close thsi for now. As this is kind of a mess

@rullzer rullzer closed this Jan 14, 2019
@rullzer rullzer deleted the techdep/noid/typeIContainer branch January 14, 2019 10:27
@snyk-bot snyk-bot mentioned this pull request Jul 18, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees
No one assigned
Labels
2. developing Work in progress technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rullzer @ChristophWurst @MorrisJobke