Closed (fixed)
Project:
Drupal core
Version:
9.0.x-dev
Component:
configuration system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Feb 2020 at 04:59 UTC
Updated:
4 Mar 2020 at 12:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
hardik_patel_12 commentedKindly review a patch.
Comment #3
hardik_patel_12 commentedComment #5
swatichouhan012 commentedComment #6
swatichouhan012 commentedComment #8
longwaveThis needs removing, this cannot be NULL any more.
The constructor declaration needs updating here to remove this variable.
Comment #9
swatichouhan012 commented@longwave i have updated patch , kindly review new patch.
Comment #11
longwaveAgain 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.
Comment #12
longwaveComment #14
himmatbhatia commentedI have fixed one test fail. Kindly review the new patch file.
Comment #16
himmatbhatia commentedKindly review the new patch file.
Comment #18
swatichouhan012 commentedWe will work on #vb13febContribution.
Comment #19
himmatbhatia commentedComment #20
alexpottComment #21
alexpottWe should add a follow-up to D9 to trigger a deprecation here, remove the $id argument. This one is a bit complicated.
Comment #22
alexpottAdded @todo
Comment #23
alexpottSo 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()
Comment #24
catchThis looks good to me, but what about just splitting the exception message change out to a separate issue that we can cherry-pick back?
Comment #25
alexpottSure opened #3113633: Improve schema incomplete exception
Comment #26
longwaveThis is no longer optional nor has a default.
Comment #27
rithesh bk commentedcurrently working on it ......
Comment #28
rithesh bk commentedChanged according to the suggestion from #26 . Please find the updated patch .......Comment #29
rithesh bk commentedSorry to update the proper comment number from the previous patch file . Changed according to the suggestion from #26 . Please find the updated patch .......
Comment #30
longwaveUnfortunately it looks like this didn't get uploaded correctly? #25 and #29 are identical and the interdiff is also empty.
Comment #31
andypostFixed comment also found that
\Drupal\Core\Config\ExtensionInstallStorage::$installProfileused inunsetbut it could be NULL according phpdoc, but constructor defines it as not null now... probably this also needs fixComment #32
berdirIt'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.
Comment #33
gábor hojtsyLet'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.
Comment #34
gábor hojtsy(Other than that the patch looks all good to me btw).
Comment #35
alexpott@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.
Comment #37
gábor hojtsySuperb, thanks!
Comment #38
himmatbhatia commented