Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Noticed by @alexpott in #2340667-45: Protect Drupal\Core\Language\Language::id, and use getId()
+++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
@@ -70,8 +70,10 @@ public function testGetInfo() {
- $language = $this->getMock('\Drupal\Core\Language\Language');
- $language->id = $this->randomMachineName();
+ $values = array('id' => $this->randomMachineName());
+ $language = $this->getMockBuilder('\Drupal\Core\Language\Language')
+ ->setConstructorArgs(array($values))
+ ->getMock();
This is not a real mock :) Since we don't actually mock any functions might as well just create a language object. If we want to use mocks then we should mock the interface and the getId() method. However this is out-of-scope for this change and should be dealt with in a new issue.
Proposed resolution
mock the interface and the getId() method
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | (novice) | Instructions | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
No.
API changes
No.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2355543-9.patch | 2.04 KB | ebeyrent |
#6 | 2355543-6.patch | 2.1 KB | ebeyrent |
#5 | 2355543.patch | 2.22 KB | ebeyrent |
#4 | language-mock-changed-to-interface-2355543-4.patch | 808 bytes | lhangea |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
XanoComment #3
YesCT CreditAttribution: YesCT commentedComment #4
lhangea CreditAttribution: lhangea commentedPatch by cheops90
Comment #5
ebeyrent CreditAttribution: ebeyrent commentedComment #6
ebeyrent CreditAttribution: ebeyrent commentedFixed a duplicate expects()
Comment #9
ebeyrent CreditAttribution: ebeyrent commentedFixed mock declaration
Comment #10
YesCT CreditAttribution: YesCT commented@cheops90 Did you have trouble getting logged into your drupal.org account?
@ebeyrent Thanks for taking a look at this issue.
Please add a comment explaining the different approach you took from #4
Also, note that interdiffs are useful. For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
Comment #11
martin107 CreditAttribution: martin107 commentedHello ebeyrent, hope you don't mind... I just want to see what the testbot has to say about your patch
so I am bumping the status to "Needs review"
Comment #12
ebeyrent CreditAttribution: ebeyrent commentedI took a slightly different approach for two reasons.
First, while there's only one test method currently, there's bound to be others in the future, so having a fixture seemed more appropriate to me than creating the language interface mock directly in the test method.
The second reason is that it follows the same convention already in place in the unit test - there are already several other mocks that are created in the setUp(), and so it made sense to follow that approach.
What do you guys think?
Comment #13
skipyT CreditAttribution: skipyT commented@YesCT: cheops90 had issues with uploading the patch, cause he doesn't has the not a spammer role yet. he applied for one and lhangea uploaded the patch file instead of him.
Comment #14
YesCT CreditAttribution: YesCT commented@skipyT OK. Please link to the webmasters issue requesting the not a spammer role for @cheops90
[edit: I ask because I did not find it in https://www.drupal.org/project/issues/webmasters ]
Also, you are all welcome in #drupal-contribute on irc.freenode.net https://www.drupal.org/irc
Comment #15
cheops90 CreditAttribution: cheops90 commentedI think the @ebeyrent patch have almost the same approach with my patch, but his advantage how already he said is to use the same LanguageInterface mock in future methods.
So i think we should go with @ebeyrent patch.
Comment #16
jhedstrom#9 looks good. This is allowed under beta phase because it cleans up internal unit test code.
Comment #17
alexpottCommitted c1bc910 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.