[RFC] Rewrite the picker implementation #950

aschempp · 2017-07-11T09:47:07Z

As discussed in several issues, the current picker implementation does not really work. Here's my rewrite to support all current features and some more!

TODO:

Improve method names
Fix coding style / docblocks
Add necessary interfaces
Add unit tests

leofeyer · 2017-07-11T10:01:22Z

Please revert the naming and namespace changes. Otherwise it is impossible to understand the changes.

leofeyer · 2017-07-11T10:04:03Z

src/Resources/contao/templates/backend/be_tinyNews.html5

@@ -24,6 +33,7 @@ setTimeout(function() {
      }
      editor.on('focus', function(){ Backend.getScrollOffset(); });
    },
+    file_browser_callback_types: <?= json_encode($pickerTypes) ?>,


The whole logic needs to be moved to the DataContainer class, so we can use $this->pickerTypes in the templates.

Is tinyMCE initialized by DataContainer ?

Yes: https://github.com/contao/core-bundle/blob/master/src/Resources/contao/classes/DataContainer.php#L537-L551

Changed in 1f24de0

leofeyer · 2017-07-11T10:05:18Z

src/Resources/contao/classes/DataContainer.php

@@ -510,7 +514,19 @@ protected function row($strPalette=null)
 			}
 		}

-		$objWidget->wizard = $wizard;
+		if ($wizard != '')


How is this change related to the picker?

It hides the picker icon and spacing if the dcaPicker is not supported (due to permissions etc.)

leofeyer · 2017-07-11T11:27:24Z

src/Resources/public/core.js

@@ -898,7 +898,7 @@ var Backend =
 		var opt = options || {},
 			maxWidth = (window.getSize().x - 20).toInt(),
 			maxHeight = (window.getSize().y - 192).toInt();
-		if (!opt.id) opt.id = 'tl_select';
+		if (!opt.id) opt.id = 'tl_listing';


You must not change this, because it is not backwards compatible with the old PageTree and FileTree widgets.

Toflar · 2017-07-12T12:07:01Z

src/Picker/PickerBuilder.php

+    /**
+     * {@inheritdoc}
+     */
+    public function createFromJson($json)


I would actually add createFromUrlValue() and saveForUrlValue() or something like this. You can keep the others if you like but this would allow us using gzencode() etc. in these methods (or anything else in the future) to transparently and backwards compatible improve the size of the content :)

implemented in a25b132, including Gzip compression

Toflar · 2017-07-12T15:05:14Z

src/Resources/contao/drivers/DC_Table.php

-					{
-						$arrRoot = $this->eliminateNestedPages($arrRoot);
-					}
+					$arrRoot = array_intersect($arrRoot, $this->Database->getChildRecords($GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['root'], 'tl_page'));


This should be $this->strTable not tl_page right?

Fixed in 368b144

Toflar · 2017-07-13T15:26:58Z

src/Controller/BackendController.php

@@ -224,8 +226,23 @@ public function alertsAction()
     */
    public function pickerAction(Request $request)


Can we have some extended description in the docblock here? :) Handles the picker redirect. does not really help a lot.

Improved in 9e53148

Toflar · 2017-07-13T15:29:38Z

src/Picker/AbstractPickerProvider.php

+     * @param FactoryInterface      $menuFactory
+     * @param TokenStorageInterface $tokenStorage
+     */
+    public function __construct(FactoryInterface $menuFactory, TokenStorageInterface $tokenStorage)


I prefer having setter injection for the TokenStorageInterface because getUser() is absolutely optional for an AbstractPickerProvider. I think we should have setTokenStorage() and getTokenStorage(). The getUser() already considers the null case anyway :)

I think this makes sense.

Changed in 37b9d7c

Toflar · 2017-07-13T15:33:07Z

src/Picker/DcaPickerProviderInterface.php

+namespace Contao\CoreBundle\Picker;
+
+/**
+ * Interface for DCA picker providers.


I would like to have more description here. Use this if ..... or something. This would help very much.

Improved in 9e53148

Toflar · 2017-07-13T15:37:05Z

src/Picker/PickerConfig.php

+     *
+     * @return string
+     */
+    public function urlEncode($gzip = true)


I would name this $compress = true. Who cares if this is gzip.

Improved in 9e53148

Toflar · 2017-07-13T15:40:55Z

src/Resources/contao/drivers/DC_Folder.php


-		if (isset($arrFilter['files']) && $arrFilter['files'] === false)
+		if (is_array($attributes))


I'd prefer a null check as there can only be array or null.

Improved in 9e53148

Toflar · 2017-07-13T15:43:30Z

src/Resources/contao/drivers/DC_Table.php

+	{
+		$attributes = parent::initPicker($picker);
+
+		if (is_array($attributes))


Check for null here and do an early return please. I hate these 5-level nestings 😄

Improved in 9e53148

leofeyer · 2017-07-14T11:48:33Z

src/Picker/ArticlePickerProvider.php

+    /**
+     * {@inheritdoc}
+     */
+    public function convertDcaValue(PickerConfig $config, $value)


We are actually converting the picked value here, aren't we?

We're converting the value from DC/database to the one we can pick.

I still prefer convertValue() or convertPickedValue(). 😄

You know the method is (only) part of the DcaPickerProviderInterface, right? That's why I named the methods so they are specific to the DCA, if we ever later add similar features for something else…

I just think that convertPickedValue() is better understandable for everyone implementing the interface. convertDcaValue() sounds like the load_callback of the field is executed to convert the DCA value. 😉

Ok, first, all the Dca parts in the method names are unnecessary in my opinion, because the class already has Dca in it.

But second, the first two methods actually read data from the DCA, so using Dca in the method name is kind of correct. The third method, however, reads user input, so …

You mean the interface does?
The convertDcaValue does not really read user input, it's basically a save_callback on the DCA value before it is rendered as a picker radio/checkbox value.

Of course it reads user input. The user picks page ID 12, submits the value and gets back {{link_url::12}}.

Again, I think that convertPickedValue() is much better understandable.

That is not correct. There is no Ajax request anymore, the values are converted before the user selects them in the picker. This also results in an instant update of the field value, contrary to the previous implementation which took seconds to update 😉

Ah, why don't you say that right away? Then convertDcaValue is actually fine. 😄

leofeyer · 2017-07-14T11:52:19Z

src/Picker/PickerConfig.php

+    /**
+     * {@inheritdoc}
+     */
+    public function jsonSerialize()


The method says JSON serialize but it returns an array?

That's how the \JsonSerializable interface works 😉

leofeyer · 2017-07-14T11:53:01Z

src/Picker/PickerConfig.php

+     */
+    public function urlEncode($gzip = true)
+    {
+        $data = json_encode($this);


Why are you encoding $this here? Should it be $this->jsonSerialize()?

That's how the \JsonSerializable interface works 😉

This is actually because json_encode() can handle objects and not because of the interface. 😉

Well json_encode will realize the object implements \JsonSerializable and uses that method…

leofeyer · 2017-07-14T11:54:19Z

src/Picker/PickerInterface.php

+     *
+     * @return PickerProviderInterface|null
+     */
+    public function getCurrentProvider();


I would prefer getProvider() here.

It returns the currently active provider (based on the value).

Then I prefer getActiveProvider().

It's named after isCurrent, which is based on KnpMenu (as described below). Still wanna change it?

leofeyer · 2017-07-14T11:54:30Z

src/Picker/PickerInterface.php

+     *
+     * @return string
+     */
+    public function getCurrentUrl();


I would prefer getUrl() here.

It returns the URL for the current provider (based on the value).

leofeyer · 2017-07-14T11:54:55Z

src/Picker/PickerProviderInterface.php

+     *
+     * @return bool
+     */
+    public function isCurrent(PickerConfig $config);


I would prefer isActive() here.

I've named this after the current variable of the KnpMenu…

Hm, ok. We should either say current or active everywhere for consistency. So getCurrentProvider(), getCurrentUrl(), isCurrent() etc. or getActiveProvider(), getActiveUrl(), isActive() etc.

I prefer the latter, but as long as it is consistent, I can live with it.

Yeah, sort of agree. We name it active most of the time in Contao. But we cannot change KnpMenu, and the isCurrent() method is also called in https://github.com/aschempp/contao-core-bundle/blob/37b9d7caa43b196d8b234d59440c1b09f3842e06/src/Picker/AbstractPickerProvider.php#L64 to determine the current menu item…

Just use "current" everywhere.

👍 so nothing to change 😎

leofeyer · 2017-07-14T11:56:46Z

src/Resources/contao/classes/DataContainer.php


 			// Deprecated since Contao 4.0, to be removed in Contao 5.0
 			$objTemplate->language = \Backend::getTinyMceLanguage();

 			$updateMode = $objTemplate->parse();

-			unset($file, $type);
+			unset($file, $type, $pickerBuilder);


Also unset $fileBrowserTypes?

Added in 9e53148

leofeyer · 2017-07-14T11:58:53Z

src/Resources/contao/controllers/BackendMain.php

-			$this->Template->main .= $this->getBackendModule(\Input::get('do'));
+			$picker = null;
+
+			if (isset($_GET['picker']))


Using isset($_GET) cannot be "overwritten" via Input::setGet(). I know that I have been doing it, too – maybe we should discuss it and be consistent?

As far as I know, this will work fine. Input::setGet removes caches and adds the variable to $_GET (see https://github.com/contao/core-bundle/blob/master/src/Resources/contao/library/Contao/Input.php#L326)

You are right. 👍

leofeyer · 2017-07-14T12:00:07Z

src/Resources/contao/drivers/DC_Folder.php

@@ -2667,7 +2662,7 @@ protected function generateTree($path, $intMargin, $mount=false, $blnProtected=t
 			}
 		}

-		if ($this->blnHideFiles)
+		if (!$this->blnFiles && !$this->blnFilesOnly)


Does this really work without $this->strPickerField &&?

Yes it does, because $blnFiles is initialized to true and only set to false on initPicker.

leofeyer · 2017-07-14T12:03:04Z

src/Resources/contao/templates/backend/be_main.html5

@@ -94,7 +94,7 @@

  <?php if ($this->isPopup): ?>
    <script>
-      if ($$('.tl_tree_checkbox,.tl_tree_radio').length > 0) {
+      if ($$('.tl_tree_checkbox,.tl_tree_radio').length > 0 && !document.location.search.test('act=select')) {


I'd prefer to have this as an outer if condition:

if (!document.location.search.test('act=select')) { if ($$('.tl_tree_checkbox,.tl_tree_radio').length > 0)) { // … } else { // … } }

Otherwise the else condition would still be evaluated in "edit multiple" mode.

That would mean the buttons are not disabled when in select mode, which is not what we want, right?

Of course. Don't know what I was thinking. 😄

leofeyer · 2017-07-14T12:04:53Z

Already looking very good! Thank you very much @aschempp.

… exists.

leofeyer · 2017-07-24T07:40:58Z

Actually merged in 874076d.

leofeyer requested changes Jul 11, 2017

View reviewed changes

leofeyer assigned aschempp Jul 11, 2017

leofeyer added the feature label Jul 11, 2017

leofeyer added this to the 4.5.0 milestone Jul 11, 2017

leofeyer requested changes Jul 11, 2017

View reviewed changes

leofeyer modified the milestones: 4.4.2, 4.5.0 Jul 11, 2017

leofeyer added bug and removed feature labels Jul 11, 2017

aschempp mentioned this pull request Jul 11, 2017

[RFC] Rewrite picker implementation contao/news-bundle#14

Merged

leofeyer changed the title ~~Rewrite the picker implementation~~ [RFC] Rewrite the picker implementation Jul 11, 2017

aschempp mentioned this pull request Jul 12, 2017

New dcaPicker: Limit tab items #924

Closed

leofeyer closed this Jul 12, 2017

aschempp changed the base branch from hotfix/4.4.1 to master July 12, 2017 11:31

leofeyer reopened this Jul 12, 2017

leofeyer mentioned this pull request Jul 12, 2017

Auswahl "Link-Adresse" oder "Bildllink" bei Benutzer "kein Administrator" fehlerhaft contao/managed-edition#22

Closed

Toflar reviewed Jul 12, 2017

View reviewed changes

leofeyer mentioned this pull request Jul 12, 2017

4.4.1: DCA-Picker doesn't work for Custom Elements #959

Closed

Toflar reviewed Jul 12, 2017

View reviewed changes

aschempp mentioned this pull request Jul 13, 2017

unallowed download-types appear in the filepicker when inside a collapsed folder contao/core#8744

Closed

agoat mentioned this pull request Jul 13, 2017

[RFC] Feature/articletree widget #912

Closed

aschempp mentioned this pull request Jul 13, 2017

DCA picker: edit multiple records at once #881

Closed

Toflar reviewed Jul 13, 2017

View reviewed changes

leofeyer reviewed Jul 14, 2017

View reviewed changes

leofeyer requested changes Jul 14, 2017

View reviewed changes

aschempp added 2 commits July 20, 2017 11:07

Drop obsolete stuff from current implementation

b70d56c

Added new picker concept

16f99cc

Use setter injection for optional security token storage

e4fd81e

leofeyer force-pushed the picker-rewrite branch from 37b9d7c to e4fd81e Compare July 20, 2017 09:09

leofeyer added 2 commits July 20, 2017 12:21

Run the PHP-CS-Fixer.

d7c796b

Add a change log entry.

9bb55cf

m-vo mentioned this pull request Jul 20, 2017

file picker: no files could be selected, radio/checkbox missing #975

Closed

Add the unit tests.

0d83931

leofeyer force-pushed the picker-rewrite branch from 5c644eb to 0d83931 Compare July 21, 2017 15:05

leofeyer added 2 commits July 21, 2017 17:19

Fix the ContaoCoreExtensionTest class.

890e9c7

Fix the remaining tests.

3ddf78f

leofeyer force-pushed the picker-rewrite branch from 1c7a27b to 2e398c6 Compare July 21, 2017 19:17

Fix two issues with the file picker provider.

7655f00

leofeyer force-pushed the picker-rewrite branch from 2e398c6 to 7655f00 Compare July 21, 2017 19:28

This was referenced Jul 23, 2017

Implemented new picker contao/calendar-bundle#9

Merged

Implemented new picker contao/faq-bundle#4

Merged

Generate the base64 string depending on whether the gzencode function…

8d813c2

… exists.

leofeyer merged commit d9a7c8f into contao:master Jul 24, 2017

leofeyer pushed a commit that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see #950).

874076d

leofeyer pushed a commit to contao/calendar-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

49e4fd6

leofeyer pushed a commit to contao/calendar-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

f0c1e89

leofeyer pushed a commit to contao/calendar-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

1ddb70a

leofeyer pushed a commit to contao/calendar-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

a041dda

leofeyer pushed a commit to contao/faq-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

8c3262d

leofeyer pushed a commit to contao/faq-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

88f2381

leofeyer pushed a commit to contao/news-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

8e9b6df

leofeyer pushed a commit to contao/news-bundle that referenced this pull request Jul 24, 2017

Rewrite the DCA picker (see contao/core-bundle#950).

4930053

aschempp deleted the picker-rewrite branch July 27, 2017 08:58

agoat pushed a commit to agoat/contao-core-bundle that referenced this pull request Aug 1, 2017

Rewrite the DCA picker (see contao#950).

043cd35

leofeyer modified the milestones: 4.4.2, 4.4 May 14, 2019

		@@ -224,8 +226,23 @@ public function alertsAction()
		*/
		public function pickerAction(Request $request)


		if (isset($arrFilter['files']) && $arrFilter['files'] === false)
		if (is_array($attributes))

[RFC] Rewrite the picker implementation #950

[RFC] Rewrite the picker implementation #950

Conversation

aschempp commented Jul 11, 2017 • edited by leofeyer Loading

leofeyer commented Jul 11, 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

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

leofeyer Jul 17, 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

leofeyer Jul 17, 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

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 Jul 17, 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

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

leofeyer commented Jul 14, 2017

leofeyer commented Jul 24, 2017

aschempp commented Jul 11, 2017 •

edited by leofeyer

Loading

leofeyer Jul 17, 2017 •

edited

Loading

leofeyer Jul 17, 2017 •

edited

Loading

leofeyer Jul 17, 2017 •

edited

Loading