Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Remove wrong @group legacy annotations #34433

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

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

These annotations are still found on branch 5.0.
Does this mean they are wrong? Why don't they make 5.0 fail if not?

@nicolas-grekas
Copy link
Member Author

1x: Using names for buttons that do not start with a lowercase letter, a digit, or an underscore is deprecated since Symfony 4.3 and will throw an exception in 5.0 ("Button" given).

but it doesn't throw on 5.0

  1. Symfony\Component\Routing\Tests\Loader\ObjectLoaderTest::testExceptionOnNoObjectReturned
    Failed asserting that exception of type "TypeError" matches expected exception "LogicException". Message was: "Return value of Symfony\Component\Routing\Tests\Loader\TestObjectLoader::getObject() must be an object, string returned" at ObjectLoaderTest.php:111

that's the only other failure triggered by this change.

This means we should have a look twice at these two tests on master, and we should remove all the other annotations since they're not covering any deprecations.

Anyone?

@@ -62,9 +62,6 @@ public function getBadResourceStrings()
];
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be removed here and on master 👍

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

LGTM

src/Symfony/Component/Form/Tests/ButtonBuilderTest.php Outdated Show resolved Hide resolved
@fancyweb
Copy link
Contributor

@nicolas-grekas Looks like we will have to apply https://github.com/symfony/symfony/pull/34434/files#diff-40e4c90663826378950ff96dce5b445eR55 on 4.4 as well or skip the test.

@nicolas-grekas
Copy link
Member Author

Looks like we will have to apply https://github.com/symfony/symfony/pull/34434/files#diff-40e4c90663826378950ff96dce5b445eR55 on 4.4 as well or skip the test.

applied

@@ -60,8 +60,6 @@ public function getDebugModes()
}

/**
* @group legacy
Copy link
Member

Choose a reason for hiding this comment

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

Removing this was missed in #33507 where the deprecation of the intercept_redirects option was reverted.

@@ -142,8 +142,6 @@ public function getToolbarConfig()
}

/**
* @group legacy
Copy link
Member

Choose a reason for hiding this comment

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

Removing this was missed in #33507 where the deprecation of the intercept_redirects option was reverted.

nicolas-grekas added a commit that referenced this pull request Nov 20, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

Remove wrong @group legacy annotations

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

These annotations are still found on branch 5.0.
Does this mean they are wrong? Why don't they make 5.0 fail if not?

Commits
-------

8d84ac3 Remove wrong @group legacy annotations
@nicolas-grekas nicolas-grekas merged commit 8d84ac3 into symfony:4.4 Nov 20, 2019
@nicolas-grekas nicolas-grekas deleted the wrong-legacy branch November 20, 2019 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.