Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Hey @tarrinneal @stuartmorgan

As you can see CI is failing because of an analyzer issue in packages that depends on video_player package.

When I run the local analyzer this is the output:

Running for packages/camera/camera...
Running command: "flutter pub get" in /Users/gabrielokura/Desktop/Projects.nosync/packages/packages/camera/camera
Resolving dependencies...
  matcher 0.12.14 (0.12.15 available)
  material_color_utilities 0.2.0 (0.3.0 available)
  meta 1.9.0 (1.9.1 available)
  vm_service 11.2.1 (11.3.0 available)
Got dependencies!
Resolving dependencies in ./example...
Got dependencies in ./example.
`--[no-]analytics` is deprecated.  Use `--suppress-analytics` to disable analytics for one run instead.
Running command: "dart analyze --fatal-infos" in /Users/gabrielokura/Desktop/Projects.nosync/packages/packages/camera/camera
Analyzing camera...
No issues found!

But the output of CIRRUS error message is:

Warning: You are using these overridden dependencies:
! video_player 2.6.1 from path ../../video_player/video_player
Running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/camera/camera
Analyzing camera...
   info - example/lib/main.dart:1004:11 - 'VideoPlayerController.VideoPlayerController.network' is deprecated and shouldn't be used. Use VideoPlayerController.networkUrl instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
1 issue found.

Resuming, Cirrus CI is failing because the dependency video_player is being overwriten in the pipeline. Could you help me with that?

video_player dependency should be using version 2.1.4 as you can check in `packages/camera/camera/example/pubspec.yaml':

dependencies:
  camera:
    # When depending on this package from a real application you should use:
    #   camera: ^x.y.z
    # See https://dart.dev/tools/pub/dependencies#version-constraints
    # The example app is bundled with the plugin so we use a path dependency on
    # the parent directory to use the current plugin's version.
    path: ../
  flutter:
    sdk: flutter
  path_provider: ^2.0.0
  video_player: ^2.1.4

Seems that cirrus ci has a cache and his dependencies are different than the examples, is this correct?

Seems that cirrus ci has a cache and his dependencies are different than the examples, is this correct?

It's not a cache, this is a test that uses in-tree dependencies to pre-detect issues that would otherwise only show up when publishing the package.

This will need to be a two-part PR. You can add an // ignore for the deprecation warning on the failing line in this PR, and then in a follow-up PR once this is published, update the call and remove the ignore.

Unrelated but:

This PR updates VideoPlayerController network constructor to fix a bug with the datasource type.

This is a confusing description for the PR; there's no bug being fixed here. It's just an improvement to the types used in the API.

This appears to be addressing the same issue as #3510; I would encourage the two of you to collaborate on a single PR.

@gabrielokura Are you still planning on updating this to address the analysis failures?

Yes, will do it until the next week. Sorry for the delay

@stuartmorgan Now CI is failing only in the repo checks after adding the //ignore for the deprecation.
Could you override this check?

Overriding version checks: We don't need to publish the examples with the ignores, since they should be updated to use the new method once this is published.

Overriding stale tree status.

auto label is removed for flutter/packages, pr: 3513, Mergeability of pull request flutter/packages/3513 could not be determined at time of merge..

AFAICT deprecating .network doesn't help with flutter/flutter#121927 but causes a lot of cost for migrations. The dataSource will eventually be String, so what we can do better is to use Uri.parse(url).toString() for .network rather than inventing a new API to deprecate the widely used one. cc @gabrielokura @stuartmorgan

AFAICT deprecating .network doesn't help with flutter/flutter#121927

That issue is about someone passing an invalid URL to a method that requires a URL, which this transition makes literally impossible. I strongly disagree that making a class of bug impossible "doesn't help".

I also have direct evidence that it does help, because doing the same transition in the url_launcher API surface completely eliminated this entire category of issue report (e.g., issues of the form "url_launcher has a bug, because when I pass <some string that looks kind of like a URL, but isn't actually one> it doesn't work on iOS"), which used to be quite common.

but causes a lot of cost for migrations.

If someone is currently passing valid, constant URLs, or if the behavior they want is just hoping that parse can handle the arbitrary input and throwing if it can't, then the cost of migration should be trivial. If they are currently passing arbitrary untrusted content and just hoping for the best, and as a result of this change realize that they are actually responsible for deciding how to go from arbitrary input to a valid URL to meet the requirements of the API surface—requirements that have always been there, but were previously only enforced by comment— and do so in a better way that just throwing, then it might be costly, but that's the change working as intended.

The dataSource will eventually be String

This is an internal implementation detail, which is irrelevant to designing a good API.

so what we can do better is to use Uri.parse(url).toString() for .network

It is true that we could continue to enforce API requirements via comment rather than via the type system, deliberately allowing people to pass invalid data and then throwing an exception when they do, but I disagree that doing so is better. We could also have made the NNBD transition easier for clients by just marking every single parameter's type as nullable and continuing to assert when someone passed a null value, but we didn't, for exactly the same reason.