Skip to content
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

Add support for arrays containing additional database values in list views #6200

Merged
merged 1 commit into from
Jul 26, 2020

Conversation

jorrit
Copy link
Contributor

@jorrit jorrit commented Jul 14, 2020

Subject

I am trying to show additional fields in the list view. These fields are not part of the entity. Adding the field is no problem thanks to configureQuery(). However, Doctrine2 returns an array instead of an object when additional columns are present in the result set.

This was discussed previously in sonata-project/SonataDoctrineORMAdminBundle#399 and sonata-project/SonataDoctrineORMAdminBundle#501 with no result. The direction in those issues was to use a DTO. This PR does not add support for DTO's but for the arrays that Doctrine2 returns.

As mentioned briefly in #6198 I found several issues with the method SonataAdminExtension::getValueFromFieldDescription():

  • It contains an unused third parameter.
  • It is public for no reason.
  • The code block if ($fieldDescription->getAssociationAdmin()) { ... } is dysfunctional because it causes $value to be a model instance instead of a field value. Apparently nobody has noticed.

As such, I think it is best to leave this method alone for now and add functionality to renderListElement().

I am targeting this branch, because it is backwards compatible.

Changelog

### Added
- Added support for columns not belonging to the model to the list view.

### Deprecated
- Calling `SonataAdminExtension::getValueFromFieldDescription()`

To do

  • Update the documentation;

@jorrit jorrit force-pushed the additionalvalues branch 4 times, most recently from 06852ee to 2e081cc Compare July 14, 2020 11:47
@jorrit jorrit marked this pull request as ready for review July 14, 2020 11:58
core23
core23 previously requested changes Jul 14, 2020
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
@jorrit jorrit force-pushed the additionalvalues branch 4 times, most recently from 29bf540 to ff85e6a Compare July 15, 2020 07:48
docs/reference/action_list.rst Outdated Show resolved Hide resolved
docs/reference/action_list.rst Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Show resolved Hide resolved
@jorrit jorrit force-pushed the additionalvalues branch 3 times, most recently from a656cd9 to fc3af36 Compare July 15, 2020 09:30
VincentLanglet
VincentLanglet previously approved these changes Jul 15, 2020
@VincentLanglet VincentLanglet requested review from core23 and a team July 15, 2020 09:38
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/Twig/Extension/SonataAdminExtension.php Outdated Show resolved Hide resolved
@phansys phansys added the minor label Jul 15, 2020
@jorrit jorrit force-pushed the additionalvalues branch 3 times, most recently from 4519332 to 502d0ec Compare July 15, 2020 09:54
@jorrit jorrit force-pushed the additionalvalues branch 2 times, most recently from 473d900 to 2a63ac2 Compare July 15, 2020 09:58
@jorrit jorrit force-pushed the additionalvalues branch from 2a63ac2 to 19bc849 Compare July 15, 2020 10:27
@jorrit jorrit force-pushed the additionalvalues branch from 19bc849 to 9a6cd57 Compare July 15, 2020 12:49
@jorrit jorrit force-pushed the additionalvalues branch from 9a6cd57 to a750669 Compare July 15, 2020 12:50
@jorrit
Copy link
Contributor Author

jorrit commented Jul 24, 2020

@core23 / @VincentLanglet Is there anything I can do to move this forward?

@VincentLanglet
Copy link
Member

@core23 Can you review this ?

@jorrit I would say you should ping @greg0ire to dismiss the review if @core23 doesn't answer in the next day(s) ?

(BTW @greg0ire, who else has the right to dismiss a review ?)

@greg0ire
Copy link
Contributor

I don't know…

@VincentLanglet
Copy link
Member

I don't know…

I find out it was repository administrators or people with write access :)

@VincentLanglet
Copy link
Member

I think we can merge this @greg0ire :)

@greg0ire greg0ire dismissed core23’s stale review July 26, 2020 09:28

Requested changes were done

@greg0ire greg0ire merged commit 0d5aea1 into sonata-project:3.x Jul 26, 2020
@greg0ire
Copy link
Contributor

Thanks @jorrit !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants