- Issue created by @joachim
- π¬π§United Kingdom joachim
How do you envisage that would look? What would be the advantage over a service?
- πΈπͺSweden johnwebdev
Well, services are shared and ConfigImporter stores internal state (errors, validated status) etc which suggests to me that it shouldn't be a service.
- π¬π§United Kingdom joachim
Ah, yes, I hadn't thought of that!
Symfony actually allows services to not be shared, but we probably don't want to introduce that extra complexity to Drupal core services.
- π¬π§United Kingdom joachim
Updated the summary. Could you add details on how a factory class would work?
Drupal 9.4.0-alpha1 β was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule β and the Allowed changes during the Drupal core release cycle β .
Drupal 9.3.0-rc1 β was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule β and the Allowed changes during the Drupal core release cycle β .
Drupal 9.2.0-alpha1 β will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule β and the Allowed changes during the Drupal core release cycle β .
Drupal 9.1.0-alpha1 β will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule β and the Allowed changes during the Drupal 9 release cycle β .
- πΈπͺSweden johnwebdev
Here is an example with pseudo code: https://gist.github.com/johndevman/6f095d41e6d49ed0306bd9683789c905
Alternatively, you could perhaps do something similar to cache (not entirely sure if the objects are shared or not here though)
cache.entity: class: Drupal\Core\Cache\CacheBackendInterface tags: - { name: cache.bin } factory: cache_factory:get arguments: [entity] cache_factory: class: Drupal\Core\Cache\CacheFactory arguments: ['@settings', '%cache_default_bin_backends%'] calls: - [setContainer, ['@service_container']]
- π©πͺGermany donquixote
I wrote this up in a project.
Too lazy to create a patch atm :)declare(strict_types = 1); namespace Drupal\MYMODULE\Config; use Drupal\Core\Config\ConfigImporter; use Drupal\Core\Config\ConfigManagerInterface; use Drupal\Core\Config\StorageComparerInterface; use Drupal\Core\Config\TypedConfigManagerInterface; use Drupal\Core\Extension\ModuleExtensionList; use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\Extension\ModuleInstallerInterface; use Drupal\Core\Extension\ThemeHandlerInterface; use Drupal\Core\Lock\LockBackendInterface; use Drupal\Core\StringTranslation\TranslationInterface; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** * Factory for ConfigImporter. * * See https://www.drupal.org/project/drupal/issues/3123491. */ class ConfigImporterFactory { /** * Constructor. * * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher * @param \Drupal\Core\Config\ConfigManagerInterface $config_manager * @param \Drupal\Core\Lock\LockBackendInterface $lock * @param \Drupal\Core\Config\TypedConfigManagerInterface $typed_config * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * @param \Drupal\Core\Extension\ModuleInstallerInterface $module_installer * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module */ public function __construct( private EventDispatcherInterface $event_dispatcher, private ConfigManagerInterface $config_manager, private LockBackendInterface $lock, private TypedConfigManagerInterface $typed_config, private ModuleHandlerInterface $module_handler, private ModuleInstallerInterface $module_installer, private ThemeHandlerInterface $theme_handler, private TranslationInterface $string_translation, private ModuleExtensionList $extension_list_module, ) {} /** * Creates a ConfigImporter instance. * * @param \Drupal\Core\Config\StorageComparerInterface $storage_comparer * * @return \Drupal\Core\Config\ConfigImporter */ public function createConfigImporter(StorageComparerInterface $storage_comparer): ConfigImporter { return new ConfigImporter( $storage_comparer, $this->event_dispatcher, $this->config_manager, $this->lock, $this->typed_config, $this->module_handler, $this->module_installer, $this->theme_handler, $this->string_translation, $this->extension_list_module, ); } }
- π¬π§United Kingdom joachim
Cool, I was halfway through writing this while tests were running a few weeks ago. I'll compare what I had with what you have and make a MR.
Drupal core is moving towards using a βmainβ branch. As an interim step, a new 11.x branch has been opened β , as Drupal.org infrastructure cannot currently fully support a branch named
main
. New developments and disruptive changes should now be targeted for the11.x
branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule β and the Allowed changes during the Drupal core release cycle β .Drupal 9.5.0-beta2 β and Drupal 10.0.0-beta2 β were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule β and the Allowed changes during the Drupal core release cycle β .
- π¬π§United Kingdom joachim
Pushed my WIP to a fork, will start to combine with @donquixote's work later.
- Status changed to Needs work
12 months ago 4:40pm 26 September 2023 - π¬π§United Kingdom joachim
Still not finding time to work on this, but I've added the actual factory service to the Field Tools module based on @donquixote's code (with a few tweaks), so the code can be grabbed from that when someone gets round to doing more work here.
Setting to NW since there's actually some code done on this issue.
- Status changed to Needs review
11 months ago 1:05pm 10 October 2023 - Merge request !4978Issue #3123491: make it easier to instantiate ConfigImporter β (Open) created by joachim
- last update
11 months ago Custom Commands Failed - last update
11 months ago Custom Commands Failed - last update
11 months ago Custom Commands Failed - last update
11 months ago 30,390 pass, 1 fail - Status changed to Needs work
11 months ago 9:02pm 11 October 2023 - π¬π§United Kingdom joachim
I'm stuck because there's no docs on why the test fails and how to do services in core now.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Added https://www.drupal.org/project/drupal/issues/3394450 π Improve the failure message from Drupal\KernelTests\Core\DependencyInjection\AutowireTest Active
- last update
11 months ago 30,414 pass - Status changed to Needs review
11 months ago 12:08pm 17 October 2023 - Status changed to Needs work
11 months ago 1:56pm 17 October 2023 - πΊπΈUnited States smustgrave
Follow up issue has been resolved.
But could a CR be written for the new search, preferably with an example?
Also issue summary mentions more details pending?
28:03 2:38 Running- Status changed to Needs review
11 months ago 2:30pm 17 October 2023 - Status changed to RTBC
11 months ago 11:05pm 17 October 2023 - πΊπΈUnited States smustgrave
Thanks much better! cR reads well too, thanks for adding an example
- π¬π§United Kingdom joachim
Haha, it was just copy-pasted from the MR diff!
Thanks for the review :)
- last update
11 months ago 30,420 pass - last update
11 months ago 30,426 pass - last update
11 months ago 30,427 pass - last update
11 months ago 30,436 pass, 1 fail - last update
11 months ago 30,456 pass - last update
10 months ago 30,464 pass - last update
10 months ago 30,483 pass - last update
10 months ago 30,485 pass 16:27 12:36 Running- last update
10 months ago 30,488 pass - last update
10 months ago 30,511 pass - last update
10 months ago 30,516 pass - last update
10 months ago 30,528 pass - last update
10 months ago 30,551 pass - last update
10 months ago 30,560 pass - last update
10 months ago 30,602 pass - last update
10 months ago 30,605 pass - last update
10 months ago 30,606 pass - last update
10 months ago 30,675 pass - last update
10 months ago 30,676 pass 1:28 59:12 Running- last update
9 months ago 30,688 pass - last update
9 months ago 30,688 pass - last update
9 months ago 30,688 pass - last update
9 months ago 30,696 pass - last update
9 months ago 30,698 pass - last update
9 months ago 30,712 pass - last update
9 months ago 30,724 pass - last update
9 months ago 30,764 pass 1:28 57:16 Running- Status changed to Needs work
9 months ago 12:21pm 19 December 2023 - π¬π§United Kingdom alexpott πͺπΊπ
I want to avoid swapability at all costs here. Supporting alternate implementations of the ConfigImporter and StorageComparer makes me feel very uneasy. I think the idea of have a factory service is a good one as long as we make changes to show the scope is to use a
\Drupal\Core\Config\StorageComparer
object to generate a\Drupal\Core\Config\ConfigImporter
object.I wish
\Drupal\Core\Config\StorageComparerInterface
did not exist. Perhaps we can move all the docs to the StorageComparer and change the typehint on \Drupal\Core\Config\ConfigImporter::__construct() to StorageComparer|StorageComparerInterface and deprecate the interface. - π¬π§United Kingdom joachim
> I wish \Drupal\Core\Config\StorageComparerInterface did not exist. Perhaps we can move all the docs to the StorageComparer and change the typehint on \Drupal\Core\Config\ConfigImporter::__construct() to StorageComparer|StorageComparerInterface and deprecate the interface.
That sounds like a follow-up to me? Or should it happen here to prevent swappability?
Could you explain what changes you want to this MR please? I'm not clear.
- π¬π§United Kingdom alexpott πͺπΊπ
The StorageInterface deprecation is definitely a follow-up.
I think my comments on the MR were pretty clear... make the factory final and @internal and instead of typehinting on StorageComparerInterface do it on StorageComparer instead.
- Status changed to Needs review
9 months ago 3:44pm 20 December 2023 - π¬π§United Kingdom joachim
Made a follow-up: π deprecate StorageComparerInterface Active
Made the fixes.
- Status changed to Needs work
9 months ago 11:45pm 20 December 2023 - πΊπΈUnited States smustgrave
1 small comment.
For the parameters could they be typehinted or would that cause issues since they're really being moved?
- π¨πSwitzerland bircher π¨πΏ
Ah this is great! For Config Split, I made a trait which gets the services from \Drupal:: in order to avoid people trying to abuse it by swapping it etc.
But with final and the concrete classes typehinted I think it is even nicer.When reviewing I saw that obviously the constructor of the form classes changed, we remove some deprecation notice and I am wondering if we need to do that here too.. but since there is a create method I think we are fine, but it makes me wonder why it was done last the other time.
Also the change record looks good. - π¬π§United Kingdom joachim
Addressed some of the comments & rebased on 11.x.
Regarding the deprecation for constructors in the two form classes -- form classes are not part of core's API and these two classes specifically have an '@internal' tag. On top of that, given it's faffy, I'd say we should leave it.