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

Define default value on Multiselect Object Type #15871

Conversation

jan888adams
Copy link
Contributor

@jan888adams jan888adams commented Aug 31, 2023

🤖 Generated by Copilot at 6a30c5e

Summary

📝🚫♻️

This pull request refactors the multiselect data type and the select types with dynamic options to use a common interface and trait for handling options and default values. It also deprecates the MultiSelectOptionsProviderInterface and updates the documentation and the Service class accordingly.

Sing, O Muse, of the mighty code review
That simplified the select types with dynamic options
And deprecated the old interface, MultiSelectOptionsProviderInterface,
That caused confusion and errors among the developers.

Walkthrough

  • Simplify the documentation for the select types with dynamic options (link)
  • Replace the deprecated MultiSelectOptionsProviderInterface with the SelectOptionsProviderInterface for the Multiselect class (link, link, link, link, link)
  • Add the DefaultValueTrait and the default value property to the Multiselect class (link, link, link)
  • Modify the getDataForResource method of the Multiselect class to handle the default value logic and convert the array of options to a comma-separated string (link)
  • Remove an unnecessary type annotation for the $optionsProvider variable in the preSave method of the Multiselect class (link)
  • Add the doGetDefaultValue method to the Multiselect class to implement the logic for retrieving the default value from the options provider or the property (link)
  • Add a new method hasStaticOptions to the SelectOptionsProviderInterface to indicate whether the options are context-dependent or not (link)
  • Modify the OptionsProviderResolver class to trigger a deprecation warning when resolving a provider that implements the MultiSelectOptionsProviderInterface and to allow resolving a provider that implements the SelectOptionsProviderInterface for both select and multiselect modes (link)

@CLAassistant
Copy link

CLAassistant commented Aug 31, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

Review Checklist

  • Target branch (11.0 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@jan888adams jan888adams changed the title [Task] Add possibility to define default value on Multiselect Object Type #15642 [Task] Define default value on Multiselect Object Type Aug 31, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
33.3% 33.3% Duplication

@jan888adams jan888adams changed the title [Task] Define default value on Multiselect Object Type Define default value on Multiselect Object Type Aug 31, 2023
@jan888adams jan888adams marked this pull request as draft September 2, 2023 08:48
@jan888adams jan888adams marked this pull request as ready for review September 2, 2023 09:01
@jan888adams
Copy link
Contributor Author

jan888adams commented Sep 6, 2023

Would it be better to provide the option to provide multiple Default values? @kingjia90 @dvesh3

@jan888adams jan888adams marked this pull request as draft September 6, 2023 16:13
@dvesh3
Copy link
Contributor

dvesh3 commented Oct 12, 2023

@jan888adams sorry for late response. This was not in the scope for v11.1 so we'll start reviewing it for v11.2.

Would it be better to provide the option to provide multiple Default values?

yes, I would add the possibility to set multiple default values for multiselect fields. Could you please take care of it? and resolve the conflicts. thank you!

@dvesh3 dvesh3 added this to the 11.2.0 milestone Oct 12, 2023
@jan888adams jan888adams force-pushed the task/default-value-for-Multiselect-Object-Type branch from 04714ec to 6a30c5e Compare October 30, 2023 17:02
@jan888adams jan888adams marked this pull request as ready for review October 30, 2023 17:33
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

jan888adams and others added 6 commits October 31, 2023 10:44
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
@jan888adams jan888adams force-pushed the task/default-value-for-Multiselect-Object-Type branch from b34dcc7 to 6081017 Compare October 31, 2023 09:46
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@kingjia90 kingjia90 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR, and sorry for the long wait.
Overall LGTM, just found some small issues

@kingjia90
Copy link
Contributor

kingjia90 commented Mar 6, 2024

@jan888adams Do you have some free time to check my feedback on #15871 (review) ? This PR needs to be merged before next week (12th March) for the upcoming 11.2, in case i can take over.
Thank you in advance

@jan888adams
Copy link
Contributor Author

@jan888adams Do you have some free time to check my feedback on #15871 (review) ? This PR needs to be merged before next week (12th March) for the upcoming 11.2, in case i can take over. Thank you in advance

@jan888adams jan888adams closed this Mar 8, 2024
@jan888adams jan888adams reopened this Mar 8, 2024
Copy link

sonarcloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.6% Duplication on New Code

See analysis details on SonarCloud

@jan888adams
Copy link
Contributor Author

The review and finalization of the Merge Request will not be completed until Tuesday, I presume. It would be appreciated if you could finish it.

@kingjia90 kingjia90 merged commit b8627f4 into pimcore:11.x Mar 8, 2024
18 checks passed
@NiklasBr
Copy link
Contributor

NiklasBr commented Mar 12, 2024

@jan888adams this causes throws an error on update from v11.5, looks like a BC break.

Timestamp: Tue Mar 12 2024 17:16:26 GMT+0100 (centraleuropeisk normaltid)
Status: 500 | 
URL: /admin/object/tree-get-children-by-id?_dc=1710260185418
Method: GET
Message: Pimcore\Model\DataObject\ClassDefinition\Data\Multiselect::setDefaultValue(): Argument #1 ($defaultValue) must be of type array, string given, called in /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/ClassDefinition/Data.php on line 215
Trace: 
in /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/ClassDefinition/Data/Multiselect.php:125
#0 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/ClassDefinition/Data.php(215): Pimcore\Model\DataObject\ClassDefinition\Data\Multiselect->setDefaultValue('')
#1 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Classificationstore/Service.php(85): Pimcore\Model\DataObject\ClassDefinition\Data->setValues(Array)
#2 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Classificationstore/Service.php(64): Pimcore\Model\DataObject\Classificationstore\Service::getFieldDefinitionFromJson(Array, 'multiselect')
#3 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Classificationstore/Dao.php(199): Pimcore\Model\DataObject\Classificationstore\Service::getFieldDefinitionFromKeyConfig(Object(Pimcore\Model\DataObject\Classificationstore\KeyConfig))
#4 [internal function]: Pimcore\Model\DataObject\Classificationstore\Dao->load()
#5 /var/www/pimcore/vendor/pimcore/pimcore/lib/Model/AbstractModel.php(220): call_user_func_array(Array, Array)
#6 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/ClassDefinition/Data/Classificationstore.php(491): Pimcore\Model\AbstractModel->__call('load', Array)
#7 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Concrete/Dao.php(152): Pimcore\Model\DataObject\ClassDefinition\Data\Classificationstore->load(Object(App\Model\DataObject\Product), Array)
#8 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Concrete/Dao.php(69): Pimcore\Model\DataObject\Concrete\Dao->getData()
#9 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/AbstractObject.php(243): Pimcore\Model\DataObject\Concrete\Dao->getById(22963)
#10 /var/www/pimcore/vendor/pimcore/pimcore/models/DataObject/Listing/Dao.php(66): Pimcore\Model\DataObject\AbstractObject::getById(22963)
#11 [internal function]: Pimcore\Model\DataObject\Listing\Dao->load()
#12 /var/www/pimcore/vendor/pimcore/pimcore/lib/Model/AbstractModel.php(220): call_user_func_array(Array, Array)
#13 /var/www/pimcore/vendor/pimcore/admin-ui-classic-bundle/src/Controller/Admin/DataObject/DataObjectController.php(144): Pimcore\Model\AbstractModel->__call('load', Array)
#14 /var/www/pimcore/vendor/symfony/http-kernel/HttpKernel.php(181): Pimcore\Bundle\AdminBundle\Controller\Admin\DataObject\DataObjectController->treeGetChildrenByIdAction(Object(Symfony\Component\HttpFoundation\Request), Object(Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher))
#15 /var/www/pimcore/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#16 /var/www/pimcore/vendor/symfony/http-kernel/Kernel.php(197): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#17 /var/www/pimcore/vendor/symfony/runtime/Runner/Symfony/HttpKernelRunner.php(35): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#18 /var/www/pimcore/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\HttpKernelRunner->run()
#19 /var/www/pimcore/public/index.php(19): require_once('/var/www/pimcor...')
#20 {main}

Changing the method to public function setDefaultValue(array|string|null $defaultValue): static and the property to public null|array|string $defaultValue = null; fixes it.

Index: models/DataObject/ClassDefinition/Data/Multiselect.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/models/DataObject/ClassDefinition/Data/Multiselect.php b/models/DataObject/ClassDefinition/Data/Multiselect.php
--- a/models/DataObject/ClassDefinition/Data/Multiselect.php
+++ b/models/DataObject/ClassDefinition/Data/Multiselect.php	(date 1710260869815)
@@ -75,7 +75,7 @@
     /**
      * @internal
      */
-    public ?array $defaultValue = null;
+    public array|string|null $defaultValue = null;
 
     public function getOptions(): ?array
     {
@@ -122,7 +122,7 @@
         return $this->renderType;
     }
 
-    public function setDefaultValue(array $defaultValue): static
+    public function setDefaultValue(array|string|null $defaultValue): static
     {
         $this->defaultValue = $defaultValue;
 

kingjia90 added a commit that referenced this pull request Mar 14, 2024
…ced by #15871 (#16779)

* remove parent interface is bc break

* fix bc-break reported by Niklas

* Update models/DataObject/ClassDefinition/Data/Multiselect.php

Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>

* Update models/DataObject/ClassDefinition/Data/Multiselect.php

Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>

* missing default handling for editmode

* missing default handling for editmode

* enrich definition

* enrich definition

* enrich definition

* Update models/DataObject/ClassDefinition/Data/Multiselect.php

Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>

* Fix SelectionProviderTrait (#16782)

* Fix SelectionProviderTrait

* Change unneeded getOptions() call by string

* fix getDataForQueryResource

---------

Co-authored-by: Sebastian Blank <sebastian.bl@gmx.de>
Co-authored-by: Sebastian Blank <blank@data-factory.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Add possibility to define default value on Multiselect Object Type
7 participants