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

Added support for deprecating aliases #24707

Closed
wants to merge 8 commits into from

Conversation

@j92
Copy link
Contributor

commented Oct 27, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24507
License MIT
Doc PR symfony/symfony-docs#...

This pr adds support for deprecating aliases. I applied the same logic as for services, so we can deprecate an alias like this:

<service id="alias_for_foobar" alias="foobar">
      <deprecated />
</service>

Or we can deprecate an alias providing a deprecation template:

<service id="alias_for_foobar" alias="foobar">
     <deprecated>The "%service_id%" service is deprecated.</deprecated>
</service>

I submitted a WIP pull request to get early feedback. Please let me know if I am on the right track, so I can finish the work in the various file loaders and dumpers. Docs and changelog will be updated after the feedback.

Thanks!

@greg0ire
Copy link
Contributor

left a comment

Thanks for implementing this, great job so far!

private $deprecated;
private $deprecationTemplate;
private static $defaultDeprecationTemplate = 'The "%service_id%" service is deprecated. You should stop using it, as it will soon be removed.';

This comment has been minimized.

Copy link
@greg0ire

greg0ire Oct 27, 2017

Contributor

Maybe "service alias" instead of just "service"?

"With \rs" => array("invalid \r message %service_id%"),
"With \ns" => array("invalid \n message %service_id%"),
'With */s' => array('invalid */ message %service_id%'),
'message not containing require %service_id% variable' => array('this is deprecated'),

This comment has been minimized.

Copy link
@greg0ire

greg0ire Oct 27, 2017

Contributor

"required"

@j92 j92 force-pushed the j92:deprecate-aliases branch 2 times, most recently from c709de5 to ced5f49 Oct 28, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

This PR needs to be rebase on master and target 4.1, it's too late to merge in into 3.4.
Note also that there is a big missing part (it's a non-trivial topic): the dumper container also needs to trigger the deprecation notice.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Oct 28, 2017

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2017

@nicolas-grekas Thanks for the feedback. I will add the missing part, do some more testing and target 4.1.

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

I am currently figuring out how to handle the trigger of the deprecation notice in the dumped container. Especially with deprecated private aliases, which are removed from the container in the RemovePrivateAliasesPass compiler pass. So after compiling, I have no reference to these aliases anymore.

For public aliases I had the idea to deprecate the actual definition in the ReplaceAliasByActualDefinitionPass compiler pass, but I'm also not sure if thats the correct way to go.

@greg0ire Maybe you have an idea that will push me in the right direction here?

@greg0ire

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

Especially with deprecated private aliases, which are removed from the container in the RemovePrivateAliasesPass compiler pass. So after compiling, I have no reference to these aliases anymore.

Not sure if this is an issue. IMO, the deprecations should be triggered when compiling the container, because that is when they are used. You don't want deprecations to be triggered for every unrelated http requests.

For public aliases I had the idea to deprecate the actual definition in the ReplaceAliasByActualDefinitionPass compiler pass, but I'm also not sure if thats the correct way to go.

If you do that, people will not be able to get rid of the deprecation by referencing the actual service definition, will they?

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

@greg0ire Sorry for the late reply, I didn't have a lot of time to work on this issue.

Are you sure that the deprecations should be triggered when compiling the container? As far as I can see and understand it, the notices are added in the dumper classes like the PhpDumper and the XmlDumper. Then the dumped container is loaded and the real errors are triggered in the ContainerBuilder. So every time the container is loaded, the deprecation error is triggered, which makes it possible to show it in the Symfony Profile for example.

@greg0ire

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

That's my point. Wouldn't that make a lot of deprecations, irrelevant to the current page? Not sure what's best.

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

That makes a lot of deprecations indeed, but that's also how it's done for services at the moment. Deprecating a service, triggers a notice on every request. So my guess is that we want the same behaviour for service aliases right?

@greg0ire

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

Correct. Go with it then.

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

Okay, I will then.

To come back to my original question, I am wondering about the comment of @nicolas-grekas :

Note also that there is a big missing part (it's a non-trivial topic): the dumper container also needs to trigger the deprecation notice.

Is it enough to implement the deprecation notices in the several dumpers like the PhpDumper and the XmlDumper, so that when they are loaded, the ContainerBuilder triggers the deprecation notice? Or I am missing other places which should trigger the deprecation notice as well?

@kiler129

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

Ok, I think there is a time to take a dust from this issue ;) First of all I don’t believe it’s enough to implement them in dumpers, this will lead to poor DX.

I spent some time with the code and I have few ideas. Two steps are necessary regardless of the solution choosen:

  1. Rebase onto master
  2. Modify ReplaceAliasByActualDefinition to ignore not just public aliases but also deprecated ones

Then we get into interesting part - how to trigger deprecation on runtime?

  • [A] Change aliases array to contain deprecation flag
    • We change aliastName => (string)target to aliasName => [target, (bool)isDeprecated]
    • We change internal yet public $this->aliases (correct me if I’m wrong?).
    • This requires modification of all calls to $this->aliases[$id] to use $this->aliases[$id][0]
    • Generally I will say it's no-no solution due to a real BC break
  • [B] Save two maps side by side
    • First map stays as it is in $this->aliases
    • Second map has structure exactly like $this->aliases but it’s checked after normal map and triggers deprecation notice as needed
    • This requires additional element and changes in base container class. Instead of having something like this (example from get()):
      if (isset($this->aliases[$id])) {
          $id = $this->aliases[$id];
      }
      we will end up with:
      if (isset($this->aliases[$id])) {
          $id = $this->aliases[$id];
      } elseif (isset($this->aliasesDeprecated[$id])) {
          $id = $this-> aliasesDeprecated[$id];
          @trigger_error(...);
      }
    • This solution has almost no BC break. The only small thing I see is we no longer guarantee that $this->aliases contains all not inlined aliases, but personally I'm not concerned
    • I don't like this solution since it introduces additional overhead to every lookup when service is not an alias nor a deprecated alias
    • This solution doesn't let us give a good DX by suggesting a replacement
  • [C] Save deprecation map
    • Map in $this->aliases stays as is
    • We add list under $this-> aliasesDeprecated containing an array with alias name as the key and target alias as value (which gives us option to provide suggestion of new service/alias name)
    • Again we have to change lookups in base container. The lookup will look like so:
      if (isset($this->aliases[$id])) {
          $id = $this->aliases[$id];
          if (isset($this->aliasesDeprecated[$id])) {
              @trigger_error(...);
          }
      }
    • This solution has almost zero BC break
    • I slightly dislike it. It's better than [B] since we get overhead only on call to aliases, but the overhead is added to all aliased - deprecated or not...
  • [D] Generate alias methods
    • Modify PhpDumper::addMethodMap() to include methods for deprecated aliases (this of course has to be done after targets)
    • Skip deprecated aliases in PhpDumper::addAlises()
    • Modify (scary) PhpDumper::addService() to generate method like so:
      protected function get[AliasName]AliasTo[TargetName]() {
          @trigger_error(...);
      
          return get[TargetName]();
      }
    • This will be hard to implement (at least form my point of view)
    • This will introduce performance loss only on deprecated aliases, nothing else

WDYT @nicolas-grekas? @fabpot?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

@kiler129 thanks for the heads up. I think we should go for D (no need for a special name for the method, the existing "generateMethodName" logic is good enough), but of course only for public aliases that cannot be inlined. Thus, the current state of the PR looks good to me. It just needs to handle two more cases:

  • non-inlined public deprecated aliases should generate a method like suggested here (D). That'd mean they'd be listed in the "methodMaps" property, and not in "aliases" anymore of course.
  • we need a new logic at compilation time that would trigger a deprecation whenever a deprecated symbol is referenced (either an alias or a service, doesn't matter).

As a general design note, I don't think it matter that the symbol being deprecated is an alias or a service: that's an implementation detail of the service graph. What matters is that a symbol that is being planned for removal triggers a warning when referenced/used. This may mean that we don't need to put the word "alias" in the messages, and use "service" all the time. WDYT?

@j92 does that make sense for you? Ok to continue?

@kiler129

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

I think we're on the right track.

  • For inlined aliases (so in practice all private ones, right?) we will get a deprecation notice during build and it will stay in compilation log which is easily accessible now
  • Non-inlined ones will become pseudo-services carrying method triggering deprecation and listed in map and will not be present in aliases array
  • For the reference logic across compilation process, if I understood your intention correctly @nicolas-grekas, you want to trigger deprecation everywhere deprecated symbol is used. I quickly searched DIC component code and found possible targets:
    • Compiler\AnalyzeServiceReferencesPass::getDefinitionId
    • Compiler\AutoAliasServicePass
    • Compiler\DecoratorServicePass
    • Compiler\ResolveReferencesToAliasesPass::getDefinitionId

After thinking for a while I believe the idea of treating services and aliases equally is right - it's an implementation detail after all. Other messages in DIC component (except obvious places like hasAlias()) also doesn't handle aliases in any special way in terms of messages.

@j92 Are you willing to rebase onto master and implement additional logic discussed?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

ResolveReferencesToAliasesPass looks like the good place to me for now

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

@kiler129 Thanks for dusting of this issue;) Yes, I am willing to rebase this branch and will start to implement the additional logic asap.

@j92 j92 changed the base branch from 3.4 to master Jan 26, 2018

@j92 j92 force-pushed the j92:deprecate-aliases branch from ced5f49 to 3bffe61 Jan 26, 2018

@kiler129

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

@j92: Any news? Sorry for poking you, but I just periodically visit PRs which are relevant to what I do ;)

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

@nicolas-grekas Can you review this one again?

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2018

@greg0ire @kiler129 Sorry for not finishing this PR in time. I got stuck a while ago and was not progressing at all. Unfortunately the next few weeks I won't have free time, so:

Is there someone who can takeover the work?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

It's ok @j92 :). Let's just finish this PR when you have time. I'm here because we just had a use-case for this in DoctrineBundle. But, no need to rush - we're after the 4.1 feature freeze. But, yes, let's definitely try to get this in there for 4.2!

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

@@ -346,8 +346,13 @@ private function parseDefinition($id, $service, $file, array $defaults)
}
foreach ($service as $key => $value) {
if (!in_array($key, array('alias', 'public'))) {
if (!in_array($key, array('alias', 'public', 'deprecated'))) {
throw new InvalidArgumentException(sprintf('The configuration key "%s" is unsupported for the service "%s" which is defined as an alias in "%s". Allowed configuration keys for service aliases are "alias" and "public".', $key, $id, $file));

This comment has been minimized.

Copy link
@artursvonda

artursvonda Aug 2, 2018

Contributor

Should the message be updated, too? Now it includes hardcoded keys for service aliases are "alias" and "public".

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@nicolas-grekas I think we need your input here :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Continued in #29968, thank you @j92!

nicolas-grekas added a commit that referenced this pull request Jan 25, 2019

feature #29968 [DI] Added support for deprecating aliases (j92, Renan)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] Added support for deprecating aliases

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes
| Fixed tickets | #24507
| License       | MIT
| Doc PR        | TBD

This PR is a continuity of #24707

Commits
-------

6c571ad Added support for deprecating aliases (runtime+dumper)
0eb071b Added support for deprecating an alias

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.