Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Theme!ThemeNegoti...
The documentation says:
The only method this service needs to implement is determineActiveTheme.
However it's not true because the service needs to implement another interface method "applies".
Comment | File | Size | Author |
---|---|---|---|
#16 | correct_documentation-2676026-15.patch | 1.38 KB | subharanjan |
#15 | correct_documentation-2676026-15.patch | 1.47 KB | zaurav |
#13 | correct_documentation-2676026-13.patch | 1.47 KB | Girish-jerk |
#10 | interdiff-2676026-8-10.txt | 1.13 KB | dimaro |
#10 | correct_documentation-2676026-10.patch | 1.47 KB | dimaro |
Comments
Comment #2
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedCould this be a first approach?
Comment #3
Chi CreditAttribution: Chi commentedI would move this sentence to determineActiveTheme method documentation.
Comment #4
jhodgdonThanks 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!
Comment #5
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedApply 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.
Comment #6
jhodgdonUm, 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).
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!
needs comma before "like" and another one after "one".
Actually we don't normally start @return docs with "Return...". Please fix.
Comment #7
jhodgdonComment #8
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThanks @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.)
Comment #9
jhodgdonUm. 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...
So please leave out the "The only methods..." sentence. Replace it by saying that your service class needs to implement this interface.
comma should be after "one" not before.
Comment #10
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpps... sorry.
I hope this is more correct.
Comment #11
jhodgdonThanks!
Comment #12
catchCan we use something other than 'kick in' that would be friendlier for people with English as an additional language?
Comment #13
Girish-jerk CreditAttribution: Girish-jerk at UniMity Solutions Pvt Limited commentedHi Catch,
I have recreated the patch #10. Done the changes mentioned in #12
Comment #14
jhodgdonThanks for the new patch! Still needs a bit of work though:
There should not be a comma at the end of this line. Sorry for not noticing this earlier.
I think that "commit" isn't the right word here.
Maybe "be used" would be better?
Comment #15
zauravUpdated patch from #13 to include suggestion from #14
Comment #16
subharanjan CreditAttribution: subharanjan commentedComment #17
subharanjan CreditAttribution: subharanjan commented@zaurav , I am sorry !! I didn't refresh the page before uploading the patch. Please ignore mine. Thanks
Comment #18
jhodgdonThese patches seem to be about the same. Either one is fine.
Setting version to 8.2.x so they get tested against that.
Comment #19
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI have put the test against 8.2.x
(Patch on #16)
Comment #20
jhodgdonThanks. 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.
Comment #21
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpps! I did not know that.
Thanks.
Comment #22
alexpottCommitted 9fb6ec3 and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!