Add AbstractAdmin::configureExportFields extension point #6348
Conversation
Codecov Report
@@ Coverage Diff @@
## 3.x #6348 +/- ##
============================================
+ Coverage 77.50% 77.52% +0.02%
- Complexity 2602 2605 +3
============================================
Files 142 142
Lines 7778 7783 +5
============================================
+ Hits 6028 6034 +6
+ Misses 1750 1749 -1
Continue to review full report at Codecov.
|
core23
force-pushed
the
export-extension
branch
from
dc8e44e
to
aae0811
Compare
| /** | ||
| * @return string[] | ||
| */ | ||
| protected function configureExportFields(): array |
There was a problem hiding this comment.
This method and signature does not follow the same idea as the other configure: parameter with the initial config and empty body
There was a problem hiding this comment.
This is not the rule for array https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AbstractAdminExtension.php#L92
core23
force-pushed
the
export-extension
branch
2 times, most recently
from
c9c572e
to
3f54f9e
Compare
There was a problem hiding this comment.
In my projects, I generally override the getExportFields method this way:
public function getExportFields()
{
return ['foo', 'bar'];
}
In these cases, the call
$this->getModelManager()->getExportFields($this->getClass());
Is useless.
This example makes me think that it should be move in the configure method.
Moreover configureExportFields is returning an array in the AdminExtension https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AbstractAdminExtension.php#L92
And should we add a word in the changelog/upgrade note to say to developers to move the logic from getExportFields to configureExportFields since the first one will be final ?
No other We're inconsistent in the handling of array values This method use references parameters:
While these methods use return values:
IMHO we should remove the referenced parameters |
core23
force-pushed
the
export-extension
branch
from
3f54f9e
to
e521562
Compare
core23
force-pushed
the
export-extension
branch
from
e521562
to
3c813d3
Compare
|
IMO return values in IMHO we should remove the referenced parameters and return values in this methods. |
When the argument is an object, it's always a reference, so there is no need for returning something. When the argument is an array, it seems more natural to me to return this array instead of using reference. |
|
Some times ago I present my idea where For methods from #6348 (comment) they should be protected with public getters too. |
I like the idea of an |
If we use an ExportMapper (similar to formMapper and showMapper), we'll need an ExportField. But I think we should wait for 4.0 before trying to make this development, in order to not delay anymore the next major. Anyway, this should not be a blocking point for your PR. |
Agree, we should release next major version for AdminBundle as fast as it is possible. This |
|
So can we merge this, also we will refactor this in the next major release? |
|
Is it ok for you @jordisala1991 ? :) |
|
Thanks @core23 |
Subject
All extension points are protected and start with
configure*. This PRs adds a new one for export fields, so you can define custom fields and can use admin extensions.I am targeting this branch, because this feature is BC.
Changelog