Problem/Motivation

#1942490: Make Twig engine available during install makes CoreBundle::registerTwig() public and static so that it can be reused in the installer. However, this is different from other methods on the CoreBundle and there's no indication of why, so someone might unwittingly try to revert this in the future.

Proposed resolution

Add a paragraph to the method docblock explaining why it is public and static.

CommentFileSizeAuthor
#10 document_corebundle_registertwig-1957042-10.patch651 byteskevin morse
#4 document_corebundle_registertwig-1957042-4.patch570 bytesfoopang
#2 document_corebundle_registertwig-1957042-2.patch571 bytesfoopang

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Novice
foopang’s picture

foopang commented
Status: Active » Needs review
StatusFileSize
new document_corebundle_registertwig-1957042-2.patch571 bytes
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -384,6 +384,8 @@ protected function registerRouting(ContainerBuilder $container) {
    * Registers Twig services.
+   * ¶
+   * This method is public and static so that it can be reused in the installer.

Bit of trailing whitespace here.

Other than that, this is probably all we need! Thanks @foopang.

foopang’s picture

foopang commented
Status: Needs work » Needs review
StatusFileSize
new document_corebundle_registertwig-1957042-4.patch570 bytes

Removed trailing whitespace and rerolled the patch. Thanks xjm for review.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Yup, looks good. Thanks @foopang!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

There's another patch that hits the CoreBundle.php file and is marked "avoid commit conflicts"... and it might have some bearing on this issue? In any case I'd wait to commit this patch until
#1939660: Use YAML as the primary means for service registration
is taken care of.

chx’s picture

chx commented

Yes, and in fact it adds a similar line although not the same, Once that's in we can freely wordsmith more. That patch bears the avoid commit conflicts tag because it's ahem, a bear to reroll :)

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs work

Definitely needs a reroll and perhaps the other installer comments in CoreBundle need wordsmithing too?

kevin morse’s picture

kevin morse commented
Status: Needs work » Needs review
StatusFileSize
new document_corebundle_registertwig-1957042-10.patch651 bytes

Here's a start with just that one comment changed.

This other line (below) doesn't really make sense to me either but then again I don't really know what's happening here.

As this is different during install, it needs to stay in PHP.
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 8.x.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.