Posted by webchick on September 24, 2012 at 5:01am
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | update.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | D8MI, Needs steps to reproduce, Needs tests |
Issue Summary
I got this during install of the latest 8.x version of Spark:
if (!empty($langcode)) {
$language = language_load(langcode);
}Oopsie. :P Fix is really simple (attached), but this means we lack test coverage.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| update-langcode.patch | 464 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 41,467 pass(es). | View details | Re-test |
Comments
#1
Found this bug too. Nice solution ;)
#2
Still needs tests.
#3
I can do this.
#4
I tried:
enabling ui translation
adding a language
going to the updates report and checking manually with the language in the url, for example: af/admin/reports/updates
But I'm not getting the error.
Looks like I might need to go somewhere that causes the
$text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => url('admin/reports/updates/update', array('language' => $language))), array('langcode' => $langcode));to get used.
Comments on the function say
* Returns the appropriate message text when site is out of date or not secure.*
* These error messages are shared by both update_requirements() for the
* site-wide status report at admin/reports/status and in the body of the
* notification e-mail messages generated by update_cron().
Ok. So I try: af/admin/reports/status
There is no error, and also, the link is generated ok.
It's also ok for admin/reports/status
I'll try adding a contrib module.
#5
I also tried:
dropping db tables
sudo rm -r sites; git checkout sites;
getting spark and putting it in modules
enabling the spark demo
no error, even when visiting admin/reports/status
enable ui translation
adding a language
going to af/admin/reports/status
no error.
I guess I need steps to reproduce. (there is some history of my env being different from webchick's .. and Sutharsan's)
#6
You might need to download an older version of a contrib module?
#7
I wont get back to this, at least until tonight.
#8
difficult bug to recreate..
_update_message_text() is only called with a value in the 4th parameter which is $langcode from update_mail()
_update_message_text() is also called from update.install but only with 3 parameters ($langcode = NULL) and therefore empty.
(see http://api.drupal.org/api/drupal/core%21modules%21update%21update.module...)
i think this test can only be triggered by a site generated email and will only ever show up in a server log file.
using dpm($langcode) before and after the patch shows that it is _always_ NULL when $langcode is included in the URL such as YesCT's testing /af/admin/reports/status
#9
Hm. Somehow it does show up in Drupal 8 Spark builds on install. Except now webchick added this patch to the build there :) If you roll it back before you install, it should show up with display errrors on hopefully.
#10
amending issue title..
#11
i could not proceed with installing Spark profile because it depends on ckeditor but there is not an 8.x version of ckeditor in the git repo (that i can locate with my novice contribution skills, sorry)
#12
whilst i agree with webchick and Gábor that this error can be reproduced by installing Spark (because the update module is part of the install profile and the entire module file is parsed for PHP errors) i still think that the _patch_ cannot be tested (during installation) because there is only ever 1 language enabled during installation and therefore !empty($langcode) will never be TRUE ?
i assume we have to find a way to trigger the site-generated email when at least 2 languages are enabled and an enabled contrib module has a newer version available. The $langcode value should be included in the url generated when these conditions are met (or however url() is handling the $language and $langcode parameters).
#13
Test setup:
i was able to generate this email message invoked from a drush script.. BEFORE applying the patch
pls see attached screenshot..
script:
#!/usr/bin/drush
$module = 'update';
$key = 'test';
$to = '##to_email##'; // NOTE: removed for privacy
$langcode = 'en-gb';
$params = array();
$from = '##from_email##'; // NOTE: removed for privacy
$send = TRUE;
drupal_mail($module, $key, $to, $langcode, $params, $from, $send);
drush_print('Test email sent to ' . $to);
#14
enabled all update notifications (not only security updates)
and applied the patch..
joates@cb200:/8.x-dev$ git status -sM core/modules/update/update.module
M sites/default/modules/devel/devel.info
joates@cb200:/8.x-dev$
and then i re-ran my drush script..
i don't think this is generating a positive test result :((
but my method may lead someone else into a more suitable way to test this?
.
#15
Anybody else can reproduce this?
#16
Nobody?
#17
Oh. The error won't happen anymore with Spark, because Spark pulls in the patch in the OP to work around it. :P
#18
With an old version of spark, what were the steps to produce then? Just install? Or did you have to go to a series of pages, which ones?
#19
It's been awhile now, but yeah iirc it was just installing and then you'd see this on the final page of the installer.
It ought to be possible to reproduce this with just core though... we just need to hit that line that has the typo, and stick that in an automated test. There are pretty decent tests in http://drupalcode.org/project/drupal.git/tree/refs/heads/8.x:/core/modul... including a function refreshUpdateStatus that lets you mock whatever scenario.
Reading through http://api.drupal.org/api/drupal/core%21modules%21update%21update.module..., looks like it happens on a module where the update is one of:
case UPDATE_UNKNOWN:
case UPDATE_NOT_CHECKED:
case UPDATE_NOT_FETCHED:
case UPDATE_FETCH_PENDING:
and the $langcode argument to that function is populated.
#20
No error for me. Tried to reproduce on a clean Spark distribution local install
Steps Taken:
#21
For me this happens during a standard install of the latest Drupal 8.x:
However, this is clearly a type-o! It's using the bare word 'langcode' as if it were a constant, however, no constant is defined so it's actually passing the string 'langcode' to that function everytime. Which isn't correct.
This patch fixes the problem for me!
#22
This affects Drupal 8.x, not just Spark.
#23
All right, I guess I'll go ahead and commit this then, since it's unlikely we make the same mistake again, but I'm leaving needs work because it's clear this portion of the code has zero test coverage.
Committed and pushed to 8.x. Heh, I totally forgot this was my patch. :P