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.
mock the interface and the getId() method
| 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 |
No.
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 commentedComment #2
xanoComment #3
yesct commentedComment #4
lhangea commentedPatch by cheops90
Comment #5
ebeyrent commentedComment #6
ebeyrent commentedFixed a duplicate expects()
Comment #9
ebeyrent commentedFixed mock declaration
Comment #10
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 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 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 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 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 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.