Problem/Motivation

Remove all deprecated code from \Drupal\Core\Config. The trickiest bit is the fallback to schema for determining which properties to export.

Proposed resolution

Remove schema fallback maintaining the exception that's is thrown when schema is missing but changing the message to be more precise about what to do next.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

Comments

Hardik_Patel_12 created an issue. See original summary.

hardik_patel_12’s picture

StatusFileSize
new7.97 KB

Kindly review a patch.

hardik_patel_12’s picture

Assigned: hardik_patel_12 » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 3112639-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

swatichouhan012’s picture

Assigned: Unassigned » swatichouhan012
swatichouhan012’s picture

Status: Needs work » Needs review
StatusFileSize
new6.22 KB
new5.28 KB

Status: Needs review » Needs work

The last submitted patch, 6: 3112639-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -196,7 +196,6 @@ class ConfigImporter {
         if ($extension_list_module === NULL) {
           $extension_list_module = \Drupal::service('extension.list.module');
         }
    

    This needs removing, this cannot be NULL any more.

  2. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
    @@ -105,10 +105,6 @@ public function __construct(StorageInterface $source_storage, StorageInterface $
    -    if ($config_manager !== NULL) {
    

    The constructor declaration needs updating here to remove this variable.

swatichouhan012’s picture

Assigned: swatichouhan012 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new7.27 KB
new5.33 KB

@longwave i have updated patch , kindly review new patch.

Status: Needs review » Needs work

The last submitted patch, 9: 3112639-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

Status: Needs work » Needs review
Related issues: +#3104307: Remove BC layers in various Drupal\Core components
StatusFileSize
new12.84 KB
new6.53 KB

Again this duplicates work in #3104307: Remove BC layers in various Drupal\Core components but it seems a bit easier to get these smaller chunks in while there are a number of unanswered questions over there.

Fixed test in #9 and added another missed deprecation. Borrowed @Berdir's approach of using an assertion in ConfigEntityType.

Status: Needs review » Needs work

The last submitted patch, 11: 3112639-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

himmatbhatia’s picture

Status: Needs work » Needs review
StatusFileSize
new13.15 KB
new482 bytes

I have fixed one test fail. Kindly review the new patch file.

Status: Needs review » Needs work

The last submitted patch, 14: 3112639-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

himmatbhatia’s picture

Status: Needs work » Needs review
StatusFileSize
new13.15 KB
new531 bytes

Kindly review the new patch file.

Status: Needs review » Needs work

The last submitted patch, 16: 3112639-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

swatichouhan012’s picture

Assigned: Unassigned » swatichouhan012
Issue tags: +vb13febContribution

We will work on #vb13febContribution.

himmatbhatia’s picture

Assigned: swatichouhan012 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new923 bytes
new12.84 KB
alexpott’s picture

Issue summary: View changes
StatusFileSize
new18.64 KB
alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
@@ -148,35 +148,23 @@ public function getPropertiesToExport($id = NULL) {
+    assert(empty($this->config_export), 'Config entities must include the config_export definition.');

We should add a follow-up to D9 to trigger a deprecation here, remove the $id argument. This one is a bit complicated.

alexpott’s picture

StatusFileSize
new742 bytes
new19.09 KB

Added @todo

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -248,8 +248,7 @@ public function toArray() {
-      $config_name = $entity_type->getConfigPrefix() . '.' . $this->id();
-      throw new SchemaIncompleteException("Incomplete or missing schema for $config_name");
+      throw new SchemaIncompleteException(sprintf("Entity type '%s' is missing 'config_export' definition in its annotation", get_class($entity_type)));

So changing an exception message is a bit out-of-the-ordinary and I think we should consider backporting this part of the patch to 8.x as it is helpful. We can do that once we've got this in.

I think the follow-up #3113620: Require `config_export` annotation for config entity types and improve ConfigEntityType::getPropertiesToExport() will end up removing this exception because it will be impossible to get to so while the exception class is not the based named - SchemaIncompleteException - it's now nothing to do with schema - I don't think it is worth the API change to change it.

If we want to do we could add a new exception here that extends from SchemaIncompleteException and give it a better name but I think we should wait for #3113620: Require `config_export` annotation for config entity types and improve ConfigEntityType::getPropertiesToExport()

catch’s picture

This looks good to me, but what about just splitting the exception message change out to a separate issue that we can cherry-pick back?

alexpott’s picture

StatusFileSize
new1.34 KB
new17.35 KB
longwave’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -42,27 +42,20 @@ class ExtensionInstallStorage extends InstallStorage {
    * @param string $collection
    *   (optional) The collection to store configuration in. Defaults to the
-   *   default collection. This parameter will be mandatory in Drupal 9.0.0.
+   *   default collection.

This is no longer optional nor has a default.

rithesh bk’s picture

Assigned: Unassigned » rithesh bk
Issue tags: -vb13febContribution +VbContribution2020

currently working on it ......

rithesh bk’s picture

Assigned: rithesh bk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new17.35 KB
new0 bytes

Changed according to the suggestion from #26 . Please find the updated patch .......

rithesh bk’s picture

StatusFileSize
new17.35 KB

Sorry to update the proper comment number from the previous patch file . Changed according to the suggestion from #26 . Please find the updated patch .......

longwave’s picture

Status: Needs review » Needs work

Unfortunately it looks like this didn't get uploaded correctly? #25 and #29 are identical and the interdiff is also empty.

andypost’s picture

Status: Needs work » Needs review
StatusFileSize
new830 bytes
new17.41 KB

Fixed comment also found that \Drupal\Core\Config\ExtensionInstallStorage::$installProfile used in unset but it could be NULL according phpdoc, but constructor defines it as not null now... probably this also needs fix

berdir’s picture

Status: Needs review » Reviewed & tested by the community

It's still possible to explicitly pass NULL as we don't have a type hint on it, that's why the BC warning explicitly checked for being passed a NULL explicitly instead of missing the argument.

I think this is OK.

gábor hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -42,27 +42,20 @@ class ExtensionInstallStorage extends InstallStorage {
-   *   (optional) The collection to store configuration in. Defaults to the
-   *   default collection. This parameter will be mandatory in Drupal 9.0.0.
+   *   The collection to store configuration in. Defaults to the default
+   *   collection.

Let's decide that we allow it to be optional, in which case "(optional)" should go back and the default documentation should stay in or not, in which case the default should not be documented :) https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... where all of this inherits from has the collection is optional.

Its mandatory but it defaults to the default collection is contradictory to me.

gábor hojtsy’s picture

(Other than that the patch looks all good to me btw).

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new775 bytes
new17.37 KB

@Gábor Hojtsy you're right it's not defaulting to anything. Let's remove the Defaults to the default collection. since it doesn't do that at all. If you pass NULL as the collection that'll be passed up the constructor... it'll probably result in the default collection being used cause of PHP's loose typing but fixing that here is out-of-scope since (a) it's is unchanged behaviour and (b) introducing strict types and scalar type hints is out-of-scope.

Removing the incorrect docs and setting back to rtbc.

  • Gábor Hojtsy committed 3ef0c67 on 9.0.x
    Issue #3112639 by alexpott, himmatbhatia, swatichouhan012, longwave,...
gábor hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, thanks!

himmatbhatia’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.