[RTM] Extended picker #755

leofeyer · 2017-04-07T15:02:41Z

This PR is the basis for an extended picker that allows to pick news, events etc. (see #7656).

It uses the KnpMenuBundle to generate the menu and dispatches the contao.build_picker_menu event so you can extend it (see http://symfony.com/doc/current/bundles/KnpMenuBundle/events.html).

TODO

Unit tests
Make the parent and list views pickable
Add a picker for news, events and maybe more (articles, FAQs)
Add a "check all" checkbox

Toflar

Looks good at first sight! Nice work! I think there are quite some adjustments needed, though 😄
General thoughts:

Lots of repetition, I think even though the DC_* classes are shit, we should not continue that pattern.
What about the old page.php and file.php picker entry points? Can we map them BC to the new picker? Or is this only for filePicker and pagePicker?
Did you think about maybe unifying that whole picker stuff? I mean right now we have pageTree, filePicker and now some picker wizard callbacks. What if this just becomes the inputType picker and all you need is configured through widget config? We could just map the old pageTree and fileTree configuration to the new picker and drop the old widgets maybe?

Toflar · 2017-04-07T15:28:11Z

src/Menu/PickerMenuBuilder.php

+            ['uri' => $this->route('contao_backend', 'files', $request)]
+        );
+
+        $item->setLinkAttribute('class', 'filemounts');


I think this should be handled by the renderer. That's what the interface method setCurrent() is for.

I suppose this comment relates to line 177, doesn't it? Because setCurrent() will not set the link attribute.

Yeah, this comment must have been moved.

Toflar · 2017-04-07T15:31:42Z

src/Resources/contao/dca/tl_content.php

@@ -1579,7 +1579,11 @@ public function tableImportWizard()
 	 */
 	public function pagePicker(DataContainer $dc)
 	{
-		return ' <a href="' . (($dc->value == '' || strpos($dc->value, '{{link_url::') !== false) ? 'contao/page' : 'contao/file') . '?do=' . Input::get('do') . '&amp;table=' . $dc->table . '&amp;field=' . $dc->field . '&amp;value=' . rawurlencode(str_replace(array('{{link_url::', '}}'), '', $dc->value)) . '&amp;switch=1' . '" title="' . StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['pagepicker']) . '" onclick="Backend.getScrollOffset();Backend.openModalSelector({\'width\':768,\'title\':\'' . StringUtil::specialchars(str_replace("'", "\\'", $GLOBALS['TL_DCA'][$dc->table]['fields'][$dc->field]['label'][0])) . '\',\'url\':this.href,\'id\':\'' . $dc->field . '\',\'tag\':\'ctrl_'. $dc->field . ((Input::get('act') == 'editAll') ? '_' . $dc->id : '') . '\',\'self\':this});return false">' . Image::getHtml('pickpage.svg', $GLOBALS['TL_LANG']['MSC']['pagepicker']) . '</a>';


I don't like that I would need to write this for every extension. I always hated that. Is there maybe a better solution?

Still asking here. This seems pretty universal now to me. Why not add an 'eval' => ['picker' => true'] setting here to ease it for devs?

Toflar · 2017-04-07T15:33:50Z

src/Resources/contao/drivers/DC_Folder.php

@@ -178,6 +178,26 @@ public function __construct($strTable)
 			$this->arrValidFileTypes = \StringUtil::trimsplit(',', strtolower($GLOBALS['TL_DCA'][$this->strTable]['config']['validFileTypes']));
 		}

+		// Configure the picker


What of this is specific to DC_Folder only? Looks like something that should be part of DataContainer? I think DataContainer should handle this in a general way and the drivers extending it should override what they want to do differently.

I have checked it and it does not really make sense to move any code into the parent class. Especially as we would have to add another abstract method setPickerValue(), which will break compatibility with all existing DCs.

Obviously I would not make it abstract. I'd just add the "normal" case to DataContainer and override the stuff that differs in the child tables :-)

Toflar · 2017-04-07T15:43:45Z

src/Resources/contao/drivers/DC_Folder.php

+			$strClass = ' picker unselectable';
+			$strPicker .= ' id="tl_select"';
+
+			if (isset($GLOBALS['TL_DCA'][$this->strTable]['config']['picker']['insertTag']))


I don't like that it's limited to insert tags. I agree that most of the times you would want to have some placeholder that is dynamically replaced by a value but it could be anything, it doesn't have to be an insert tag.

It is not limited to insert tags; the picker will return any given value.

But the JavaScript additionally supports the data-inserttag and data-callback attributes; the first one will wrap the given value in an insert tag and the second one will trigger an Ajax callback. Both are optional.

Toflar · 2017-04-07T15:44:48Z

src/Resources/contao/drivers/DC_Folder.php

@@ -2692,12 +2740,28 @@ protected function generateTree($path, $intMargin, $mount=false, $blnProtected=t
 				{
 					$return .= ' <a href="'.$this->addToUrl('&amp;act=move&amp;mode=2&amp;pid='.$currentEncoded).'" title="'.\StringUtil::specialchars(sprintf($GLOBALS['TL_LANG']['tl_files']['uploadFF'], $currentEncoded)).'">'.\Image::getHtml('new.svg', $GLOBALS['TL_LANG'][$this->strTable]['move'][0]).'</a>';
 				}
+
+				if ($this->strPickerField)


I know, a lot of code is already duplicated and it's shit anyway but does that mean we have to add even more duplicate lines? :)

I checked this, too, but there are too many small differences to extract the code into a parent method.

Hm, let me check again once everything is implemented. There might be some optimization potential after all.

I have moved most of the code to the parent class now.

Toflar · 2017-04-07T15:47:47Z

src/Resources/contao/drivers/DC_Folder.php

+		$this->arrPickerValue = $varValue;
+
+		// TinyMCE will pass the path instead of the ID
+		if (strpos($varValue[0], \Config::get('uploadPath') . '/') === 0)


This smells. If something needs a special handling this is always an indicator that it is even wrong or third party bundles might need special handling to = needs extension point.

Again, this is not new code. We are using this already:

https://github.com/contao/core-bundle/blob/master/src/Resources/contao/widgets/FileSelector.php#L625

Ok. Still ;)

I know what you mean. :)

Toflar · 2017-04-07T15:51:17Z

src/Menu/PickerMenuBuilder.php

+     *
+     * @throws \RuntimeException
+     */
+    public function createMenu(BackendUser $user)


I think passing the user is not needed at all. The ones that want to limit the menu to some permissions should check for themselves, meaning the event listeners should listen to the event and inject the security.token_storage and check for permissions there.

Also they need to set the label as stated above.

Changed in ab0ebb3.

Toflar · 2017-04-07T15:53:20Z

src/Menu/PickerMenuBuilder.php

+     *
+     * @param string $pagePickerLabel
+     */
+    public function setPagePickerLabel($pagePickerLabel)


This introduces state and has nothing to do with the picker menu builder. The file picker and page picker should both listen to the event and set their own label.

I fully agree that the service must not be stateful.

However, I think it is ok if the service adds the two core navigation items. Do you really think we need a separate event listener for that?

Yes, I think the core should not behave differently than all the other listeners. It will also prevent from stuff like "the core can do this, you cannot in the listener unfortunately".
It's separation of concerns, easier to test, easier to remove etc. pp. I mean, create an AbstractDcaListener or something that gets the url, label and css class from its child class. That way it's a breeze implementing FAQ and other regular DCA related listeners.

Changed in ab0ebb3.

Toflar · 2017-04-07T15:57:43Z

src/Resources/contao/drivers/DC_Folder.php

+				$strPicker .= ' data-inserttag="' . $GLOBALS['TL_DCA'][$this->strTable]['config']['picker']['insertTag'] . '"';
+			}
+
+			if (isset($GLOBALS['TL_DCA'][$this->strTable]['config']['picker']['callback']))


What is data-callback? Is this maybe missing in this PR? Also, I suppose it's a JS callback, can we maybe name it as such? :)

That's the data-callback attribute we are using already (since Contao 4).

https://github.com/contao/core-bundle/blob/master/src/Resources/public/core.js#L952

Ah, didn't know that, cool stuff.

jankout · 2017-04-07T21:10:58Z

Thank you very much. It is great that we have the compatibility between the Contao's extensions.

leofeyer · 2017-04-11T10:32:06Z

Ok, I have completely reworked the implementation according to what @Toflar and I have discussed. It now uses tagged services that provide the picker information and a custom route that redirects depending on the picker value. Works great. 😄

Here's how to add a provider in your custom bundle: contao/news-bundle#10

Toflar · 2017-04-11T10:56:46Z

src/Controller/BackendController.php

+     *
+     * @return Response
+     *
+     * @Route("/picker", name="contao_backend_picker")


To me, this is definitely a rather internal route. I'd prefer to have something like _contao/picker like we do for the cron.

Toflar · 2017-04-11T11:06:09Z

src/Resources/contao/dca/tl_content.php

@@ -1579,7 +1579,11 @@ public function tableImportWizard()
 	 */
 	public function pagePicker(DataContainer $dc)
 	{
-		return ' <a href="' . (($dc->value == '' || strpos($dc->value, '{{link_url::') !== false) ? 'contao/page' : 'contao/file') . '?do=' . Input::get('do') . '&amp;table=' . $dc->table . '&amp;field=' . $dc->field . '&amp;value=' . rawurlencode(str_replace(array('{{link_url::', '}}'), '', $dc->value)) . '&amp;switch=1' . '" title="' . StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['pagepicker']) . '" onclick="Backend.getScrollOffset();Backend.openModalSelector({\'width\':768,\'title\':\'' . StringUtil::specialchars(str_replace("'", "\\'", $GLOBALS['TL_DCA'][$dc->table]['fields'][$dc->field]['label'][0])) . '\',\'url\':this.href,\'id\':\'' . $dc->field . '\',\'tag\':\'ctrl_'. $dc->field . ((Input::get('act') == 'editAll') ? '_' . $dc->id : '') . '\',\'self\':this});return false">' . Image::getHtml('pickpage.svg', $GLOBALS['TL_LANG']['MSC']['pagepicker']) . '</a>';


Still asking here. This seems pretty universal now to me. Why not add an 'eval' => ['picker' => true'] setting here to ease it for devs?

Toflar · 2017-04-11T11:06:49Z

src/Resources/contao/drivers/DC_Folder.php

+		$strPicker = '';
+
+		// Add the picker attributes
+		if ($this->strPickerField)


Still a lot of repetition here. Can't we put this in its own method? :)

…the DCA.

Toflar · 2017-04-18T07:33:26Z

src/Resources/contao/dca/tl_content.php

@@ -270,7 +270,7 @@
 			'exclude'                 => true,
 			'search'                  => true,
 			'inputType'               => 'text',
-			'eval'                    => array('rgxp'=>'url', 'decodeEntities'=>true, 'maxlength'=>255, 'dcaPicker'=>true, 'fieldType'=>'radio', 'filesOnly'=>true, 'tl_class'=>'w50 wizard'),
+			'eval'                    => array('rgxp'=>'url', 'decodeEntities'=>true, 'maxlength'=>255, 'dcaPicker'=>array(), 'fieldType'=>'radio', 'filesOnly'=>true, 'tl_class'=>'w50 wizard'),


I think it should remain 'dcaPicker'=>true but optionally accepting an array.

leofeyer · 2017-04-18T07:41:34Z

@ausi We could add the following code here:

$this->setPickerValue();

$strDriver = 'DC_' . $GLOBALS['TL_DCA'][$this->strPickerTable]['config']['dataContainer'];
$objDca = new $strDriver($this->strPickerTable);
$objDca->field = $this->strPickerField;

// Set the active record
if (isset($_GET['id']) && $this->Database->tableExists($this->strPickerTable))
{
	/** @var Model $strModel */
	$strModel = \Model::getClassFromTable($this->strPickerTable);

	if (class_exists($strModel))
	{
		$objModel = $strModel::findByPk(\Input::get('id'));

		if ($objModel !== null)
		{
			$objDca->activeRecord = $objModel;
		}
	}
}

// Call the load_callback
if (is_array($GLOBALS['TL_DCA'][$this->strPickerTable]['fields'][$this->strPickerField]['load_callback']))
{
	foreach ($GLOBALS['TL_DCA'][$this->strPickerTable]['fields'][$this->strPickerField]['load_callback'] as $callback)
	{
		if (is_array($callback))
		{
			$this->import($callback[0]);
			$this->arrPickerValue = $this->{$callback[0]}->{$callback[1]}($this->arrPickerValue, $objDca);
		}
		elseif (is_callable($callback))
		{
			$this->arrPickerValue = $callback($this->arrPickerValue, $objDca);
		}
	}
}

Would this solve your problem?

ausi · 2017-04-18T10:21:44Z

@leofeyer I think this is not enough because the ID of the databse record is missing. I created the PR #763, this should be a working solution.

leofeyer · 2017-04-18T10:49:49Z

Nope, the ID is not missing. It is added by the FileTree and PageTree classes, therefore I have added the isset($_GET['id']) check.

    <p><a href="' . ampersand(\System::getContainer()->get('router')->generate('contao_backend_picker', array('do'=>'page', 'target'=>$this->strTable.'.'.$this->strField, 'value'=>implode(',', $arrSet), 'id'=>$this->activeRecord->id, 'popup'=>1))) . '" class="tl_submit" id="pt_' . $this->strField . '">'.$GLOBALS['TL_LANG']['MSC']['changeSelection'].'</a></p>

(The changes have not been pushed yet.)

ausi · 2017-04-18T10:55:50Z

But wouldn’t the $_GET['id'] conflict with the data container that shows the list/tree view (like in a news archive)? That’s why I added the ID to the target parameter in my PR.

leofeyer · 2017-04-18T11:23:46Z

Yes, you are right – as always. 😄

leofeyer · 2017-04-18T11:47:48Z

Well, you were only half way right. It id parameter was only used in the PageTree and FileTree widgets, which are both not switchable, which is why the problem did not occur.

However, your changes in line 484 now always add the ID, which is a different behavior than before. I don't think it will break anything, but we should review it carefully.

ausi · 2017-04-18T12:00:37Z

src/Resources/contao/classes/DataContainer.php

+		if (!isset($GLOBALS['TL_DCA'][$this->strPickerTable]['fields'][$this->strPickerField]))
+		{
+			throw new InternalServerErrorException('Target field "' . $this->strPickerTable . '.' . $this->strPickerField . '" does not exist.');
+		}


This if needs to be moved to the end of the method, because the DCA onload_callback may add the field dynamically.

Changed in d86a642.

ausi · 2017-04-18T12:38:55Z

src/Resources/public/core.js

@@ -941,20 +941,24 @@ var Backend =
 				alert('Could not find the SimpleModal frame');
 				return;
 			}
-			ul = frm.document.getElementById(opt.id);
+			ul = frm.document.getElementById('tl_select');


This change broke an existing URL picker (similar to the old one from tl_content) in my test. Without this change the picker works as expected, but with it the {{link_url::*}} insert tag around the ID is missing.

Hm, the core is not passing an id attribute anymore, because you are supposed to pass a callback function instead. Therefore, tl_select is kind of a convention.

Is it crucial that we support opt.id here, too?

I think it’s very likely that somebody copied the tl_content::pagePicker() method to use it for an extension or in a project, this would include \'id\':\'' . $dc->field . '\', too. Not supporting opt.id is be a BC break I think.

How about using tl_select as a fallback?
frm.document.getElementById(opt.id || 'tl_select')

That's possible of course, but it does not really help. Do we want to support opt.id in Contao 5 as well? Then we should encourage people to provide it. Or do we want to use tl_select in Contao 5? Then we should add a deprecation notice if someone uses opt.id.

Do we want to support opt.id in Contao 5 as well?

I think it’s OK to deprecate it.

Changed in 2b43a70.

Description ----------- As discussed in contao/contao#745 (comment) If no image size is selected or configured the image should not get modified by Contao and the original image should be used instead. An exception to this rule is if the image has an EXIF orientation tag, in this case the image gets rotated. Commits ------- f33ebafb Enable skipIfDimensionsMatch for empty configs 3903b17e CS fixes

leofeyer added 3 commits April 7, 2017 13:55

Use the file manager instead of the file picker.

94f133b

Use the site structure instead of the page picker.

2b83e79

Always show the selected nodes.

89e1ea9

leofeyer added the feature label Apr 7, 2017

leofeyer added this to the 4.4.0 milestone Apr 7, 2017

leofeyer self-assigned this Apr 7, 2017

Add the menu builder.

5d1443a

leofeyer force-pushed the feature/dca-picker branch from 8dbb75b to 5d1443a Compare April 7, 2017 15:09

leofeyer mentioned this pull request Apr 7, 2017

Verlinkung zu News und Events (Entwurf) contao/core#7656

Closed

Toflar suggested changes Apr 7, 2017

View reviewed changes

leofeyer added 3 commits April 10, 2017 15:21

Use a separate event listener to add the core picker items.

ab0ebb3

Allow to add arbitrary picker attributes.

05bd5c8

Add an abstract parent class for all menu builder listener.

e69b2cc

leofeyer changed the title ~~[RFC] Extended picker~~ [WIP] Extended picker Apr 11, 2017

leofeyer force-pushed the feature/dca-picker branch 3 times, most recently from aa472d6 to a759b51 Compare April 11, 2017 09:52

Use providers instead of events.

8a6e13c

leofeyer force-pushed the feature/dca-picker branch from a759b51 to 8a6e13c Compare April 11, 2017 09:55

Enable the picker for the list view.

2dcd211

leofeyer changed the title ~~[WIP] Extended picker~~ [RFC] Extended picker Apr 11, 2017

leofeyer mentioned this pull request Apr 11, 2017

Register the news picker contao/news-bundle#10

Merged

Toflar suggested changes Apr 11, 2017

View reviewed changes

leofeyer added 2 commits April 11, 2017 15:24

Optimize the code.

093b68b

Add the „dcaPicker“, „dcaPickerDo“ and „dcaPickerIcon“ eval flags to …

61065b7

…the DCA.

leofeyer force-pushed the feature/dca-picker branch from af43885 to d3cab12 Compare April 11, 2017 19:26

Adjust the file and page tree widgets and the TinyMCE picker dialog.

2dd757b

leofeyer force-pushed the feature/dca-picker branch from d3cab12 to 2dd757b Compare April 11, 2017 19:31

leofeyer added 2 commits April 17, 2017 17:36

Adjust the PluginTest class.

5b224d3

Update the unit tests.

d683c39

leofeyer force-pushed the feature/dca-picker branch from dbd99f3 to d465dec Compare April 17, 2017 15:55

Fix the tests on Windows systems.

c7e5aa3

leofeyer force-pushed the feature/dca-picker branch from d465dec to c7e5aa3 Compare April 17, 2017 16:01

Toflar reviewed Apr 18, 2017

View reviewed changes

Also support 'dcaPicker'=>true if no options need to be configured.

83dcc22

ausi mentioned this pull request Apr 18, 2017

[RFC] Followup #755 Extended picker #763

Closed

Trigger the onload_callback when initializing the picker.

896a5f9

ausi requested changes Apr 18, 2017

View reviewed changes

Move the picker field check to the end of the routine.

d86a642

ausi reviewed Apr 18, 2017

View reviewed changes

Use opt.id to find the root element in the modal window.

2b43a70

ausi approved these changes Apr 19, 2017

View reviewed changes

leofeyer merged commit 81da4a0 into develop Apr 21, 2017

leofeyer added a commit that referenced this pull request Apr 21, 2017

Add the DCA picker (see #755).

927894b

leofeyer deleted the feature/dca-picker branch April 21, 2017 19:32

leofeyer mentioned this pull request Apr 22, 2017

Adding a ArticleTree widget (like PageTree) #747

Closed

leofeyer added a commit that referenced this pull request Apr 27, 2017

Replace the switch=1 option with a context parameter (see #755).

727e144

leofeyer added a commit to contao/calendar-bundle that referenced this pull request Apr 28, 2017

Add the DCA picker (see contao/core-bundle#755).

63c872b

leofeyer added a commit to contao/faq-bundle that referenced this pull request Apr 28, 2017

Add the DCA picker (see contao/core-bundle#755).

3ab652b

ausi mentioned this pull request Jun 19, 2017

Change yellow radio buttons in dcaPicker #884

Closed

leofeyer mentioned this pull request Jun 11, 2020

Teaser-Element #1417

Closed

[RTM] Extended picker #755

[RTM] Extended picker #755

Conversation

leofeyer commented Apr 7, 2017 • edited Loading

TODO

Toflar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankout commented Apr 7, 2017

leofeyer commented Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofeyer commented Apr 18, 2017 • edited Loading

ausi commented Apr 18, 2017

leofeyer commented Apr 18, 2017

ausi commented Apr 18, 2017

leofeyer commented Apr 18, 2017

leofeyer commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofeyer commented Apr 7, 2017 •

edited

Loading

leofeyer commented Apr 11, 2017 •

edited

Loading

leofeyer commented Apr 18, 2017 •

edited

Loading