Skip to content

IServerContainer interface should extend IContainer#32502

Merged
DeepDiver1975 merged 1 commit into
masterfrom
servercontainer_inheritance_fix
Nov 7, 2018
Merged

IServerContainer interface should extend IContainer#32502
DeepDiver1975 merged 1 commit into
masterfrom
servercontainer_inheritance_fix

Conversation

@patrickjahns

Copy link
Copy Markdown
Contributor

Description

While bumping phan to the 1.0.x branch - several issues have been caught.
Amongst is the fact, that our Applications have access to the ServerContainer via a IAppContainer

* @return \OCP\IServerContainer

This method returns a IServerContainer - which has specific services defined - but not general query method.

Some applications, then proceed to use query on the object they have received - which during RunTime is https://github.com/owncloud/core/blob/bc84acd48053b314100f101c975100bc220adc9a/lib/private/Server.php

But this is not correct - the interface doesn't provide the method and we are relying on the implementation here.

I propose to state, that IServerContainer infact extends IContainer so we are conform with allowing query methods.

Alternative - be strict about and rewrite the apps that rely on this implementation detail to use only allowed methods from the interface

Related Issue

Fixes the issues found by phan:

[php-phan:L55:86s] apps/files_external/lib/AppInfo/Application.php:47 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query
[php-phan:L56:86s] apps/files_external/lib/AppInfo/Application.php:50 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query
[php-phan:L57:86s] apps/files_external/lib/AppInfo/Application.php:53 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query
[php-phan:L58:86s] apps/files_external/lib/AppInfo/Application.php:56 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query
[php-phan:L59:86s] apps/files_external/lib/AppInfo/Application.php:59 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query
[php-phan:L60:86s] apps/files_external/lib/AppInfo/Application.php:62 PhanUndeclaredMethod Call to undeclared method \OCP\IServerContainer::query

Motivation and Context

Solve technical debt, prevent future bugs

How Has This Been Tested?

  • 🤖

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 makes sense

@PVince81 PVince81 added this to the development milestone Aug 29, 2018
@patrickjahns

Copy link
Copy Markdown
Contributor Author

@PVince81 - don't feel comfortable with merging straight away - would love to have input from @butonic and @DeepDiver1975

I am sure there was a initial reasoning why we limited the scope initially

@codecov

codecov Bot commented Aug 29, 2018

Copy link
Copy Markdown

Codecov Report

Merging #32502 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #32502   +/-   ##
=========================================
  Coverage     64.11%   64.11%           
  Complexity    18641    18641           
=========================================
  Files          1176     1176           
  Lines         70185    70185           
  Branches       1270     1270           
=========================================
  Hits          45002    45002           
  Misses        24813    24813           
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.89% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.39% <ø> (ø) 18641 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e55e530...d1555ec. Read the comment docs.

@PVince81

Copy link
Copy Markdown
Contributor

Seems they were both added in #4459 but somehow not linked together. My impression is that if might have been forgotten...

This was the time where the app framework was still living in a separate app and that PR is when the app framework code got merged into core.

@DeepDiver1975
DeepDiver1975 merged commit a3bd931 into master Nov 7, 2018
@DeepDiver1975
DeepDiver1975 deleted the servercontainer_inheritance_fix branch November 7, 2018 21:55
@lock lock Bot locked as resolved and limited conversation to collaborators Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants