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 a universal table picker #714

Merged
merged 24 commits into from
Dec 13, 2019
Merged

Add a universal table picker #714

merged 24 commits into from
Dec 13, 2019

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Sep 3, 2019

Replaces contao/core-bundle#1171 and contao/core-bundle#999

It's basically the same implementation as by @qzminski. It also adds a picker for articles and content elements in the respective content elements.

Example configuration:

$GLOBALS['TL_DCA']['tl_module']['fields']['article'] = [
    'label' => &$GLOBALS['TL_LANG']['tl_module']['article'],
    'exclude' => true,
    'foreignKey' => 'tl_article.title',
    'inputType' => 'picker',
    'eval' => [
        //'context' => 'event', // optional, use a custom picker provider
        //'multiple'    => true, // switch to checkbox selection
        //'orderField'     => 'articleOrder',
    ],
    'sql' => ['type' => 'integer', 'unsigned' => true],
];

Contrary to contao/core-bundle#1171, the foreignKey attribute is used to generate the value from the regular DCA callbacks. If that's not the desired result, implement a custom widget that takes over the rendering.

FYI this does not mean that any DCA table can be picked, it still always needs a valid picker provider. Now added a universal picker provider that can select from any DCA table 🎉

@aschempp aschempp added this to the 4.9 milestone Sep 3, 2019
@aschempp aschempp self-assigned this Sep 3, 2019
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

qzminski
qzminski previously approved these changes Sep 4, 2019
Copy link
Member

@qzminski qzminski left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few minor details that could be improved but without an impact on the picker itself.

@leofeyer
Copy link
Member

As briefly discussed in Slack with @aschempp, we could add an universal picker provider for all pickers that return a database ID.

@aschempp
Copy link
Member Author

@leofeyer I think we can merge this if there's no more feedback. I'll create a separate PR for the universal picker provider.

@aschempp
Copy link
Member Author

PR updated with a universal picker provider to pick any DC record 🎉

core-bundle/src/Picker/UniversalPickerProvider.php Outdated Show resolved Hide resolved
core-bundle/src/Picker/UniversalPickerProvider.php Outdated Show resolved Hide resolved
core-bundle/src/Picker/UniversalPickerProvider.php Outdated Show resolved Hide resolved
core-bundle/src/Picker/UniversalPickerProvider.php Outdated Show resolved Hide resolved
core-bundle/src/Picker/UniversalPickerProvider.php Outdated Show resolved Hide resolved
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

LGTM except for missing tests. I think they might influence the outcome again so I'll approve once this PR is completely ready.

Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

I like the latest changes, however, the AbstractDataContainerPickerProvider class does not really make sense to me. It contains a lot of code that is only required for DC_Table and that would have to be overwritten to make it work with other DCAs.

I would prefer not to have an abstract class. Just rename it to TablePickerProvider and we are all set.

@aschempp
Copy link
Member Author

I like the latest changes, however, the AbstractDataContainerPickerProvider class does not really make sense to me. It contains a lot of code that is only required for DC_Table and that would have to be overwritten to make it work with other DCAs.

How come you think that? Every custom DC I ever implemented did extend DC_Table and adjusted some fields. I don't think it's better to extend the TablePickerProvider, which would be the case if there's no abstract.

@leofeyer
Copy link
Member

You should not extend the TablePickerProvider for a folder picker provider obviously.

An abstract class should contain code that is used by 90% of their child classes. In this case, however, only about 20% of the code is relevant for a folder picker, which indicates that the abstract class is too concrete. 🤷‍♂

@leofeyer leofeyer merged commit d5f8580 into master Dec 13, 2019
@leofeyer leofeyer deleted the feature/universal-picker branch December 13, 2019 11:31
@leofeyer
Copy link
Member

Thank you @aschempp.

@leofeyer leofeyer changed the title Universal picker Add a universal table picker Jan 10, 2020
@fritzmg
Copy link
Contributor

fritzmg commented May 31, 2021

@aschempp I am trying to gather some documentation for this. However I noticed some discrepancies with the original description in this issue. If I try to implement a news picker like this:

// contao/dca/tl_content.php
use Contao\CoreBundle\DataContainer\PaletteManipulator;

$GLOBALS['TL_DCA']['tl_content']['fields']['news'] = [
    'label' => ['News', 'Choose a news article.'],
    'foreignKey' => 'tl_news.headline',
    'inputType' => 'picker',
    'eval' => ['tl_class' => 'clr'],
    'sql' => ['type' => 'integer', 'unsigned' => true, 'default' => 0],
];

PaletteManipulator::create()
    ->addField('news', 'text', PaletteManipulator::POSITION_AFTER)
    ->applyToPalette('text', 'tl_content')
;

Then the following error will occur:

Exception:
The table name must not be empty

  at vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\DcaLoader.php:50
  at Contao\DcaLoader->__construct()
  at ReflectionClass->newInstanceArgs()
     (vendor\contao\contao\core-bundle\src\Framework\ContaoFramework.php:177)
  at Contao\CoreBundle\Framework\ContaoFramework->createInstance()
     (vendor\contao\contao\core-bundle\src\Picker\AbstractTablePickerProvider.php:133)
  at Contao\CoreBundle\Picker\AbstractTablePickerProvider->supportsContext()
     (vendor\contao\contao\core-bundle\src\Picker\PickerBuilder.php:94)
  at Contao\CoreBundle\Picker\PickerBuilder->supportsContext()
     (vendor\contao\contao\core-bundle\src\Resources\contao\widgets\Picker.php:138)
  at Contao\Picker->generate()
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Widget.php:653)
  at Contao\Widget->generateWithError()
     (vendor\contao\contao\core-bundle\src\Resources\contao\templates\backend\be_widget.html5:3)
  at include('vendor/contao/contao/core-bundle/src/Resources/contao/templates/backend/be_widget.html5')
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\TemplateInheritance.php:100)
  at Contao\Widget->inherit()
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Widget.php:600)
  at Contao\Widget->parse()
     (vendor\contao\contao\core-bundle\src\Resources\contao\classes\DataContainer.php:670)
  at Contao\DataContainer->row()
     (vendor\contao\contao\core-bundle\src\Resources\contao\drivers\DC_Table.php:2003)
  at Contao\DC_Table->edit()
     (vendor\contao\contao\core-bundle\src\Resources\contao\classes\Backend.php:648)
  at Contao\Backend->getBackendModule()
     (vendor\contao\contao\core-bundle\src\Resources\contao\controllers\BackendMain.php:167)
  at Contao\BackendMain->run()
     (vendor\contao\contao\core-bundle\src\Controller\BackendController.php:49)
  at Contao\CoreBundle\Controller\BackendController->mainAction()
     (vendor\symfony\http-kernel\HttpKernel.php:157)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (vendor\symfony\http-kernel\HttpKernel.php:79)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (vendor\symfony\http-kernel\Kernel.php:195)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (web\index.php:31)
  at require('web/index.php')
     (web\app.php:4)

However if I then add a relation with table then everything is fine. I can even remove foreignKey - in fact the foreignKey declaration does not seem to have an effect whatsoever.

// contao/dca/tl_content.php
use Contao\CoreBundle\DataContainer\PaletteManipulator;

$GLOBALS['TL_DCA']['tl_content']['fields']['news'] = [
    'label' => ['News', 'Choose a news article.'],
    'inputType' => 'picker',
    'eval' => ['tl_class' => 'clr'],
    'sql' => ['type' => 'integer', 'unsigned' => true, 'default' => 0],
    'relation' => ['type' => 'hasOne', 'load' => 'lazy', 'table' => 'tl_news'],
];

PaletteManipulator::create()
    ->addField('news', 'text', PaletteManipulator::POSITION_AFTER)
    ->applyToPalette('text', 'tl_content')
;

Is this correct?

@aschempp
Copy link
Member Author

aschempp commented Jun 2, 2021

hmm. Did you build the internal cache? The relation table should be taken from the DcaExtractor data not the DCA, but that might be a bug …

@fritzmg
Copy link
Contributor

fritzmg commented Jun 2, 2021

No, the error was taken from the dev environment, without an internal cache being built.

@aschempp
Copy link
Member Author

aschempp commented Jun 2, 2021

Can you try if it works with the cache? Then maybe that's the problem…

Anyway, looks like it deserves a bug report to me 😉

@fritzmg
Copy link
Contributor

fritzmg commented Jun 2, 2021

Right, but seeing as it works just with the relation... what advantages would the foreignKey definiton bring? Is it supposed to work with either or?

@aschempp
Copy link
Member Author

aschempp commented Jun 2, 2021

The table relations fall back to foreignKey, so our picker should do that as well.

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.

None yet

5 participants