Merge pull request #713 from contao-community-alliance/hotfix/label-callback-arguments

label callback arguments

Author: zonky2

src/Contao/Callback/ModelLabelCallbackListener.php

@@ -45,6 +45,12 @@ public function getArgs($event)
             $event->getModel()->getPropertiesAsArray(),
             $event->getLabel(),
             new DcCompat($event->getEnvironment(), $event->getModel()),
+            // Contao hands the prepared label fields over as a fourth argument, and its own
+            // callbacks declare them as required - "tl_member::addIcon()" is one of several.
+            // Without it, every table carrying such a callback was fatal here. Callbacks that
+            // declare only three parameters stay unaffected; PHP passes the surplus argument
+            // and they never look at it.
+            $event->getArgs(),
         ];
     }
 
@@ -53,17 +59,20 @@ public function getArgs($event)
      */
     public function update($event, $value)
     {
-        $groupingInformation = ViewHelpers::getGroupingMode($event->getEnvironment());
-        if (
-            isset($groupingInformation['mode'])
-            && ($groupingInformation['mode'] === GroupAndSortingInformationInterface::GROUP_NONE)
-        ) {
-            if (!\is_array($value)) {
-                return;
-            }
+        if (\is_array($value)) {
             /** @var list<string> $value */
 
-            $this->updateTableMode($event, $value);
+            // An array means the callback handed the label fields back - regardless of how the
+            // list happens to be grouped. Tying this to the table mode dropped the return value
+            // everywhere else, which is why a Contao callback left the label empty.
+            $groupingInformation = ViewHelpers::getGroupingMode($event->getEnvironment());
+            $isTableMode         =
+                isset($groupingInformation['mode'])
+                && ($groupingInformation['mode'] === GroupAndSortingInformationInterface::GROUP_NONE);
+
+            $this->updateArguments($event, $value, $isTableMode);
+
+            return;
         }
 
         if (!\is_string($value)) {
@@ -100,31 +109,42 @@ private function updateNonTableMode(ModelToLabelEvent $event, ?string $value): v
     }
 
     /**
-     * Set the value in the event.
+     * Take the label fields a callback returned over into the event.
      *
-     * @param ModelToLabelEvent   $event     The event being emitted.
-     * @param string|list<string> $arguments The label arguments.
+     * @param ModelToLabelEvent   $event       The event being emitted.
+     * @param string|list<string> $arguments   The label arguments.
+     * @param bool                $byPosition  Whether numeric keys may be matched by position.
      *
      * @return void
      */
-    private function updateTableMode(ModelToLabelEvent $event, array|string $arguments): void
+    private function updateArguments(ModelToLabelEvent $event, array|string $arguments, bool $byPosition): void
     {
         if (empty($arguments)) {
             return;
         }
 
         $updateArguments = $event->getArgs();
 
-        // Step 1 update arguments by index as propertyName
-        foreach ($event->getFormatter()->getPropertyNames() as $index => $propertyName) {
+        // By name - this always means the same field on both sides.
+        foreach ($event->getFormatter()->getPropertyNames() as $propertyName) {
             if (!isset($arguments[$propertyName])) {
                 continue;
             }
 
             $updateArguments[$propertyName] = $arguments[$propertyName];
         }
 
-        // Step 2 update arguments by index as integer
+        // By position - only where both sides describe the same list of columns, which is the
+        // table mode. Anywhere else the two have nothing to do with each other: a picker shows
+        // the one property it was configured with, while a Contao callback counts along its own
+        // "label/fields" - "tl_member" reserves the first of those for the icon and would push
+        // that markup into the picker's only column, replacing the value one is meant to pick.
+        if (!$byPosition) {
+            $event->setArgs($updateArguments);
+
+            return;
+        }
+
         foreach ($event->getFormatter()->getPropertyNames() as $index => $propertyName) {
             if (!isset($arguments[$index])) {
                 continue;

src/Contao/View/Contao2BackendView/TreePicker.php

@@ -1435,9 +1435,13 @@ public function loadCollection($rootId = null, $level = 0, $providerName = null)
      * Retrieve the formatter for the given model.
      *
      * @param ModelInterface $model    The model for which the formatter shall be retrieved.
-     * @param bool           $treeMode Flag if we are running in tree mode or not.
+     * @param bool           $treeMode Flag if we are running in tree mode or not. No longer read -
+     *                                 a configured label now counts in either mode - but kept so
+     *                                 that overriding methods keep their signature.
      *
      * @return ModelFormatterConfigInterface
+     *
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
     protected function getFormatter(ModelInterface $model, $treeMode)
     {
@@ -1450,12 +1454,12 @@ protected function getFormatter(ModelInterface $model, $treeMode)
 
         $listing = $backendView->getListingConfig();
 
-        if ($listing->hasLabelFormatter($model->getProviderName())) {
-            return $listing->getLabelFormatter($model->getProviderName());
-        }
-
-        // If not in tree mode and custom label has been defined, use it.
-        if (!$treeMode && $this->itemLabel) {
+        // A label the caller configured wins, in tree mode as well. Whoever puts the picker on a
+        // field knows which column names the record there - the listing config of the target
+        // table describes its own back end view instead, down to columns that are not fields at
+        // all: "tl_member" opens its list with an empty one reserved for the icon, and that is
+        // what ended up in the picker.
+        if ($this->itemLabel) {
             $label     = $this->itemLabel;
             $formatter = new DefaultModelFormatterConfig();
             $formatter->setPropertyNames($label['fields']);
@@ -1465,6 +1469,10 @@ protected function getFormatter(ModelInterface $model, $treeMode)
             return $formatter;
         }
 
+        if ($listing->hasLabelFormatter($model->getProviderName())) {
+            return $listing->getLabelFormatter($model->getProviderName());
+        }
+
         // If no label has been defined, use some default.
         $properties = [];
         foreach ($definition->getPropertiesDefinition()->getProperties() as $property) {