Needs work
Project:
Drupal core
Version:
main
Component:
configuration system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
1 Dec 2018 at 00:22 UTC
Updated:
5 May 2020 at 15:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
andypostThere's a usage of constructor in drush & config_installer so probably there's no way to make it BC compatible
Comment #3
andypostminor fix
Comment #6
andypostFix form
Comment #9
andypostComment #10
kristen polThanks for the patch.
1) In reviewing the code in the patch, I found one item that needs addressing in the code.
This should be removed.
2) I also checked the D9 dev codebase for the creation of ConfigImporter instances to see if anything was missing from the patch.
For reference:
These are the files updated in the patch:
These are the files in D9 dev code that create a ConfigImporter:
These are included in the patch:
These are not included in the patch:
Reviewing the code in
OverriddenConfigImportTest.php, it looks like this needs to be changed:$this->container->get('theme_handler'),to
$this->container->get('theme_installer'),3) Tried to apply the patch to check that theme_handler and themeHandler*/ThemeHandler* were updated as expected in those files but the patch failed so needs a re-roll
Comment #11
kristen polComment #12
kristen polComment #13
kristen polOk, done reviewing now. Sorry for the back and forth.
Comment #14
aleevasThe #6 was re-rolled
Comment #15
kristen polThanks for the re-roll... it was successful. There are still some items to address in #10 so putting back to "Needs work".
Comment #16
aleevasComment #17
aleevasKristen thank you for review.
I've tried to fix last patch.
Hope this one will be more fit
Comment #18
aleevasComment #19
kristen polThanks for the update.
1) I see the change added for ConfigSingleImportForm.php per #10 (1)
2) I see the change added for OverriddenConfigImportTest.php per #10 (2)
3) The patch applies cleanly to 9.0 and 9.1:
4) I reviewed the code and I realize there is a minor thing I didn't catch before.
The new line isn't grouped with the other Drupal\Core\Extension* lines but I see that it's alphabetical which is correct. This is probably ok since we aren't supposed to fix unrelated items in the patch.
Same as above.
I don't see any issues and patch applies cleanly, so I'll mark this RTBC, but not sure how to test this manually.
Comment #20
alexpottThis change makes tonnes of sense but we need to do in a BC compatible manor. Which will be fun.
We need to
I think we should make this change as that documents what we expect.
We need to remove the typehint completely here so we can accept both ThemeHandlerInterface and ThemeInstallInterface objects.
Here we need to detect what $theme_installer is. If it is a ThemeInstallerInterface do the assignment. If it is a ThemeHandlerInterface get the theme installer service from \Drupal::service(). If it is never we should throw an \InvalidArgumentException()
Comment #21
andypostFiled CR https://www.drupal.org/node/3133243
Also attempt to address #20
Comment #22
kristen polAssigning to myself to review.
Comment #23
kristen polThanks for the update.
1) Updated the change record slightly to improve "discoverability" and clarity:
ConfigImporter using theme installer service instead of handler
to
ConfigImporter using theme installer service instead of theme handler
2) Reviewed interdiff and see changes for #20.2 and #20.3 but some comments:
What about the docblock still using
ThemeInstallerInterface?This doesn't cover:
Also, I'm unclear if not using extra parentheses for the "not" (!) is the standard. I searched core and see both used though I find the use without parentheses confusing myself.
A few examples:
3) Patch applies cleanly.
4) Tests pass.
5) Back to "Needs work" to possibly address any of #2 above.
Comment #24
andypostRe #23.3 operator precedence in PHP is tricky but instanceof is higher priority then negate
Comment #25
kristen polThanks but mainly I'm curious if core is standardizing on one style or the other because I see both in the codebase.
Comment #26
jungleRe #25, I would vote for
if (!($blah instanceof Foo))with clear intention, other thanif (!$blah instanceof Foo), If we are going to standardize it.Yes,
isntanceofhas higher precedence than!, see https://www.php.net/manual/en/language.operators.precedence.phpComment #27
kristen polSame with me if there was a push to standardize it (assuming there isn't a standard yet).
Comment #28
andypostI see no reason to write extra confusing "()" if language dictate its rules.
It will just confuse people familiar with PHP and will provide wrong (but " more readable") examples in core
Comment #29
kristen polI'm not sure why you say it's "wrong" as it isn't technically wrong. Also, we weren't the only ones that said we prefer using the parentheses. (I asked in Slack.)
This is stylistic though, similar to using curly braces for clarity when they technically aren't required.
That said, I don't have a strong enough opinion to care which way it goes. What does bother me is when the code isn't consistent. :)
Leaving that logic as is seems fine but there are still a couple different items mentioned in #23 that could be addressed: 1) docblock question, and 2) exception that @alexpott requested. Thanks.