Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

dimaro’s picture

Status: Active » Needs review
FileSize
1.09 KB

Could this be a first approach?

Chi’s picture

Return the name of the theme, or NULL if other negotiators like the configured default one should kick in instead.

I would move this sentence to determineActiveTheme method documentation.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch and the review comment!

So since this was written, it looks like someone actually made an interface for this, which is what should have been there all along.

In which case, instead of saying essentially, in this paragraph, "These are the methods you need to implement and here is what they need to do", what we should do is just say that the service class needs to implement \Drupal\Core\Theme\ThemeNegotiatorInterface -- which gives exactly that information.

And while we are fixing this, can we instead of saying "see user.services.yml for an example", be more explicit and give the service name and the class that implements it? Thanks!

dimaro’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
1.5 KB

Apply changes mentioned on #2.

Apply changes mentioned on #4.
Here I had some doubts, I did not know how to document this in a more correct way.

jhodgdon’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -12,10 +12,9 @@
    + * (this service uses the class \Drupal\user\Theme\AdminNegotiator). The service
    

    Um, sorry I guess I wasn't clear.

    The part in parentheses needs to say something like:

    (see the xyz.whatever.whatever service in user.services.yml for an example).

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -12,10 +12,9 @@
    + * class needs to implement \Drupal\Core\Theme\ThemeNegotiatorInterface.
    

    Doh. I just realized that this documentation is on the interface itself.

    So this probably just needs to say somewhere "implementing this interface" rather than having a link to itself!

  3. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -46,7 +45,8 @@ public function applies(RouteMatchInterface $route_match);
    +   *   Return the name of the theme, or NULL if other negotiators like the
    +   *   configured default one should kick in instead.
    

    needs comma before "like" and another one after "one".

    Actually we don't normally start @return docs with "Return...". Please fix.

jhodgdon’s picture

Status: Needs review » Needs work
dimaro’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
1.41 KB

Thanks @jhodgdon and my apologies for the mistakes.
This patch fix 1 and 3 on #6.

The second point maybe could be something like this:
"Implementing this interface the only methods this service needs to implement are applies and determineActiveTheme."

(The second point has not been resolved yet. Change this point to leave it as it was on #2.)

jhodgdon’s picture

Status: Needs review » Needs work

Um. If you implement an interface, it means you need to implement all of the methods on the interface. I don't think we need to list them. It would just mean that if another method is added at a later time, the docs would be wrong. So we should just say that someone needs to implement the interface and not be more specific.

    So...

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -12,10 +12,10 @@
    + * example). The only methods this service needs to implement are applies and
    + * determineActiveTheme.
    

    So please leave out the "The only methods..." sentence. Replace it by saying that your service class needs to implement this interface.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -46,7 +46,8 @@ public function applies(RouteMatchInterface $route_match);
    +   *   default ,one should kick in instead.
    

    comma should be after "one" not before.

dimaro’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
1.13 KB

comma should be after "one" not before.

Upps... sorry.

I hope this is more correct.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
@@ -46,7 +45,8 @@ public function applies(RouteMatchInterface $route_match);
+   *   default one, should kick in instead.

Can we use something other than 'kick in' that would be friendlier for people with English as an additional language?

Girish-jerk’s picture

Status: Needs work » Needs review
FileSize
1.47 KB

Hi Catch,
I have recreated the patch #10. Done the changes mentioned in #12

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch! Still needs a bit of work though:

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -12,10 +12,9 @@
    + * To set the active theme, create a new service tagged with 'theme_negotiator',
    

    There should not be a comma at the end of this line. Sorry for not noticing this earlier.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -46,7 +45,8 @@ public function applies(RouteMatchInterface $route_match);
    +   *   The name of the theme, or NULL if other negotiators, like the configured
    +   *   default one, should commit instead.
    

    I think that "commit" isn't the right word here.

    Maybe "be used" would be better?

zaurav’s picture

Updated patch from #13 to include suggestion from #14

subharanjan’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
subharanjan’s picture

@zaurav , I am sorry !! I didn't refresh the page before uploading the patch. Please ignore mine. Thanks

jhodgdon’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +needs backport to 8.1.x, +needs backport to 8.0.x

These patches seem to be about the same. Either one is fine.

Setting version to 8.2.x so they get tested against that.

dimaro’s picture

I have put the test against 8.2.x
(Patch on #16)

jhodgdon’s picture

Thanks. Just as a note, I didn't bother to do that, because all RTBC issues are automatically tested against the version of the issue, about once every 24 hours. So I think at the first retest it would have moved to 8.2.x.

dimaro’s picture

Upps! I did not know that.
Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9fb6ec3 and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!

  • alexpott committed 6e6b09c on 8.2.x
    Issue #2676026 by dimaro, subharanjan, Girish-jerk, zaurav, jhodgdon:...

  • alexpott committed db315e9 on 8.1.x
    Issue #2676026 by dimaro, subharanjan, Girish-jerk, zaurav, jhodgdon:...

  • alexpott committed 9fb6ec3 on 8.0.x
    Issue #2676026 by dimaro, subharanjan, Girish-jerk, zaurav, jhodgdon:...

Status: Fixed » Closed (fixed)

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