Skip to content

Move deprecation under use statements #25014

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
Closed
wants to merge 2 commits into from
Closed

Move deprecation under use statements #25014

wants to merge 2 commits into from

Conversation

greg0ire
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR n/a

@greg0ire
Move deprecation under use statements
579149b 
It references one of them, and produces a wrong error message.
@greg0ire
Copy link
Contributor Author

Don't merge yet there are many more occurences.

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 18, 2017 •
edited
Loading

Files I need to check:

  • src/Symfony/Component/Config/DependencyInjection/ConfigCachePass.php
  • src/Symfony/Component/Console/Event/ConsoleExceptionEvent.php
  • src/Symfony/Component/Process/ProcessBuilder.php
  • src/Symfony/Component/Security/Core/Exception/NonceExpiredException.php
  • src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MemcacheSessionHandler.php
  • src/Symfony/Component/HttpFoundation/Session/Storage/Handler/WriteCheckSessionHandler.php
  • src/Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandler.php
  • src/Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandlerSchema.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CompilerDebugDumpPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/FormPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationExtractorPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/PropertyInfoPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslatorPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationDumperPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheClearerPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheWarmerPass.php
  • src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/SerializerPass.php
  • src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php
  • src/Symfony/Bundle/FrameworkBundle/Translation/PhpStringTokenParser.php
  • src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php
  • src/Symfony/Bundle/TwigBundle/Command/DebugCommand.php
  • src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php
  • src/Symfony/Bundle/SecurityBundle/Command/InitAclCommand.php
  • src/Symfony/Bundle/SecurityBundle/EventListener/AclSchemaListener.php

found with grep -rn trigger_error .|grep ':14:'|grep ':class'

@greg0ire
Copy link
Contributor Author

It's ready now

derrabus
derrabus reviewed Nov 18, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Command/InitAclCommand.php Outdated
@@ -11,7 +11,7 @@

namespace Symfony\Bundle\SecurityBundle\Command;

@trigger_error(sprintf('Class "%s" is deprecated since version 3.4 and will be removed in 4.0. Use Symfony\Bundle\AclBundle\Command\SetAclCommand instead.', SetAclCommand::class), E_USER_DEPRECATED);
@trigger_error(sprintf('Class "%s" is deprecated since version 3.4 and will be removed in 4.0. Use Symfony\Bundle\AclBundle\Command\SetAclCommand instead.', InitAclCommand::class), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this below the use statements and use ::class for both classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait my fix was incomplete it seems.

@greg0ire
Copy link
Contributor Author

Travis build failures seem unrelated.

@nicolas-grekas
Copy link
Member

Actually I think the convention has always been reverse: the deprecation should be just below the namespace. That's what we did in 2.7 & 2.8, and if you grep with a more generic regexp, you'll see that most of the existing notices are at the top.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Nov 19, 2017
nicolas-grekas
nicolas-grekas requested changes Nov 19, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

should really be at the very top of the file, just below the namespace

@greg0ire
Copy link
Contributor Author

@nicolas-grekas but then the use statements can't be used, see https://3v4l.org/8PjTg . I see 2 solutions:

  1. reconsider the convention
  2. refrain from using the ::class notation when referring to other classes (is that what you meant?)

@nicolas-grekas
Copy link
Member

Oh, got it! So this is a real bug fix. Let's merge as is then, no need to change everything now (and maybe discuss the general change somewhere else.)

@nicolas-grekas
Copy link
Member

Thank you @greg0ire.

nicolas-grekas added a commit that referenced this pull request Nov 19, 2017
@nicolas-grekas
bug #25014 Move deprecation under use statements (greg0ire)
eb92d7c 
This PR was squashed before being merged into the 3.4 branch (closes #25014).

Discussion
----------

Move deprecation under use statements

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | n/a

Commits
-------

0a5b016 Move deprecation under use statements
chalasr
chalasr requested changes Nov 19, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Command/InitAclCommand.php
@@ -22,6 +20,8 @@
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Schema\SchemaException;

@trigger_error(sprintf('Class "%s" is deprecated since version 3.4 and will be removed in 4.0. Use Symfony\Bundle\AclBundle\Command\InitAclCommand instead.', InitAclCommand::class), E_USER_DEPRECATED);

Copy link
Member

Choose a reason for hiding this comment

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

Change is not needed

Copy link
Member

Choose a reason for hiding this comment

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

Race condition :) not a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, let me fix it

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry, it's merged already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chalasr chalasr closed this Nov 19, 2017
This was referenced Nov 21, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr requested changes

@weaverryan weaverryan weaverryan approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@aivus aivus aivus approved these changes

@Simperfit Simperfit Simperfit approved these changes

Assignees
No one assigned
Labels
Bug Status: Needs Work
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
8 participants
@greg0ire @nicolas-grekas @weaverryan @aivus @derrabus @Simperfit @chalasr @carsonbot