[FrameworkBundle] use Table instead of the deprecated TableHelper #12970
Conversation
xabbuh
commented
Dec 13, 2014
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
|
👍 |
| $table->getStyle()->setCellHeaderFormat('%s'); | ||
| } | ||
|
|
||
| $table->render(); |
There was a problem hiding this comment.
IMO the render method should also allow to pass the output as parameter (as it has been in TableHelper). It makes way more sense and then we don't need the protected getOutput method at all.
There was a problem hiding this comment.
The current way of passing the output as constructor to the Table object makes not much sense. The table object is just a representation of a table. And a table is not bound to the ouput, just the rendering is.
There was a problem hiding this comment.
I see your point. But shouldn't we move this discussion into a new issue?
|
Any other opinions on this? |
|
Do we really need to keep the old |
|
@jakzal To me it seems unlikely that someone relies on the existing method, but strictly speaking changing its signature would be a BC break. That's why I introduced the new method. |
|
It would indeed be a BC break, but an allowed one. These classes are not part of the API. Given it's unlikely that anyone would rely on them, I don't think it would be a big deal to remove the old method. It is confusing to have |
|
We should at least deprecate the old method. |
4911e63 to
8372476
Compare
|
Indeed, |
|
It's fine to break BC here in 2.7, but do we really want to do that in 2.5? I think that nobody should have relied on this part of the code, but I just want to raise the concern once more before merging. |
|
@fabpot Do you want me to revert the last change and introduce the |
|
I think that merging it as is is the way to go. |
|
You have to fix tests (the lowest version of dependencies seems to be not ok). |
b4b67ed to
4130bff
Compare
| * | ||
| * @return OutputInterface The output | ||
| */ | ||
| protected function getOutput() |
There was a problem hiding this comment.
I think that changing the visibility of $output is enough.
There was a problem hiding this comment.
Changing the visibility makes it mutable. The methods allows us to keep the property read-only.
4130bff to
c5ad74d
Compare
|
@saro0h Thanks for the hint. I updated the dependencies to make the tests pass again. |
|
👍 |
1 similar comment
|
👍 |
|
Thank you @xabbuh. |
…bleHelper (xabbuh) This PR was merged into the 2.5 branch. Discussion ---------- [FrameworkBundle] use Table instead of the deprecated TableHelper | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- c5ad74d use Table instead of the deprecated TableHelper
|
@xabbuh Can you make a PR for 2.6? Thanks. |
|
@fabpot Of course, can you please merge the |
|
Never mind, I was able to rebase locally (see #13273). |
…ed TableHelper (xabbuh) This PR was merged into the 2.6 branch. Discussion ---------- [2.6][FrameworkBundle] use Table instead of the deprecated TableHelper | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Changes on top of #12970 for the `2.6` branch. Commits ------- 0bcb594 use Table instead of the deprecated TableHelper