Skip to content

[2.7][FrameworkBundle] Removed the use of TableHelper #13197

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
Merged
fabpot merged 1 commit into from
Jan 6, 2015
Merged

[2.7][FrameworkBundle] Removed the use of TableHelper #13197

fabpot merged 1 commit into from
Jan 6, 2015

Conversation

@saro0h
Copy link
Contributor

@saro0h saro0h commented Jan 1, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

The PR #13121 on 3.0 removed the TableHelper class. Now the tests don't pass when using components=high version of dependencis, because of the use of the deprecated TableHelper.

This one removes the use of TableHelper without the removal of the TableHelper class, and adapt the existing code to use the Table class instead.

@saro0h saro0h force-pushed the remove-use-tablehelper branch from e02c2f3 to 94cb377 Compare January 1, 2015 23:54
@saro0h
Copy link
Contributor Author

saro0h commented Jan 2, 2015

Ping @nicolas-grekas : Can you take a look at that one?

The tests on components=high pass, but now, it's the components=low that has problems. The version installed is 2.4.5 (https://travis-ci.org/symfony/symfony/jobs/45641726#L3033) where the Table that is needed does not exist… I updated the composer.json as i the console component must be at least at 2.5, what do you think about that?

@saro0h saro0h force-pushed the remove-use-tablehelper branch 3 times, most recently from 612d127 to 219f455 Compare January 2, 2015 00:42
@fabpot
Copy link
Member

fabpot commented Jan 2, 2015

@saro0h IMO, and when possible, we should remove usage of deprecated classes in the oldest possible branch.

@saro0h saro0h force-pushed the remove-use-tablehelper branch from 219f455 to 0f66a54 Compare January 2, 2015 10:21
@saro0h
Copy link
Contributor Author

saro0h commented Jan 2, 2015

I definitely can but I'll be obliged to do 2 PR (or more) because of the evolution of code between version.

Here is one exemple:

  • 2.7: TextDescriptor::describeEventDispatcherListeners() exists and uses the TableHelper
  • 2.5: The method does not exist at all.

Do you want me to do the work of removing the deprecated on all branches maintained on Symfony?

@xabbuh
Copy link
Member

xabbuh commented Jan 2, 2015

This is basically the same as #12970.

@saro0h
Copy link
Contributor Author

saro0h commented Jan 2, 2015

@xabbuh No it's not: the code changed between the two branches.

@xabbuh
Copy link
Member

xabbuh commented Jan 2, 2015

@saro0h Indeed, I missed that change. Thanks.

xabbuh
xabbuh reviewed Jan 2, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php
Copy link
Member

@xabbuh xabbuh Jan 2, 2015

Choose a reason for hiding this comment

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

You now don't make use of the decorated output anymore.

Copy link
Contributor Author

@saro0h saro0h Jan 2, 2015

Choose a reason for hiding this comment

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

Actually, in 3.0, I removed that also to make it more "standard". So I did the same here.

@saro0h saro0h force-pushed the remove-use-tablehelper branch from 0f66a54 to 852afac Compare January 4, 2015 13:05
hhamon
hhamon reviewed Jan 4, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php Outdated
Copy link
Contributor

@hhamon hhamon Jan 4, 2015

Choose a reason for hiding this comment

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

Moving this here means you will output a table with headers but without any content rows. It's not exactly the same behavior.

@saro0h saro0h force-pushed the remove-use-tablehelper branch 10 times, most recently from 7853b2a to 7f30017 Compare January 5, 2015 23:23
@hhamon
Copy link
Contributor

hhamon commented Jan 5, 2015

Looks good!

@saro0h saro0h force-pushed the remove-use-tablehelper branch from 7f30017 to adffcde Compare January 6, 2015 08:28
fabpot
fabpot reviewed Jan 6, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php Outdated
Copy link
Member

@fabpot fabpot Jan 6, 2015

Choose a reason for hiding this comment

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

looks like it's not needed

@saro0h saro0h force-pushed the remove-use-tablehelper branch from adffcde to 08a5b5a Compare January 6, 2015 08:49
@fabpot
Copy link
Member

fabpot commented Jan 6, 2015

Thank you @saro0h.

@fabpot fabpot merged commit 08a5b5a into symfony:2.7 Jan 6, 2015
fabpot added a commit that referenced this pull request Jan 6, 2015
@fabpot
minor #13197 [2.7][FrameworkBundle] Removed the use of TableHelper (s…
e7bdb5b 
…aro0h)

This PR was merged into the 2.7 branch.

Discussion
----------

[2.7][FrameworkBundle] Removed the use of TableHelper

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

The PR #13121 on 3.0 removed the ``TableHelper `` class. Now the tests don't pass when using ``components=high`` version of dependencis, because of the use of the deprecated TableHelper.

This one removes the use of ``TableHelper`` without the removal of the ``TableHelper`` class, and adapt the existing code to use the Table class instead.

Commits
-------

08a5b5a [FrameworkBundle] Removed the use of TableHelper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

FrameworkBundle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@saro0h @fabpot @xabbuh @hhamon