As per #1216094: We call too many things 'language', clean that up we are converting schemas and parallel APIs to use 'langcode' instead of 'language' where a language code is involved. The use of 'language' inconsistently for language objects and language codes in Drupal 7 keeps throwing people off the cliff and makes understanding the API much harder. The attach patch is for path module and is a possibly rough start. Let's see how it fairs with a complete test suite.

No upgrade path yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +DrupalWTF, +D8MI

Tagging.

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode.patch, failed testing.

sun’s picture

Issue tags: +API clean-up

Cleaning up bogus/duplicate "API cleanup" term.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
26.38 KB

Added update code in update.inc to fix bootstrap test failure.

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode-4.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
27.46 KB

More langcode misses in tests.

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode-6.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.13 KB

More langcode misses in locale.test and making the update conditional on existence of url_alias.

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode-8.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
29.97 KB

The path.test that breaks is under simpletest/tests, huh, this was misleading :) Fixes for those too.

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode-10.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
29.97 KB

A tiny language fallback error in drupal_lookup_path() caused many tests to fail. With a one char change there we should be in much better shape :)

Status: Needs review » Needs work

The last submitted patch, path-language-to-langcode-12.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.06 KB

The update.inc bootstrap preparation code can run in multiple requests through the update, so we need to be more specific about testing for the right conditions, the existence of the url_alias table and the language column too. Also, the index creation in the update was not correct, now it is created correctly there too. This should now pass all tests.

Gábor Hojtsy’s picture

Issue tags: +langcode

Tagging up so we can find the langcode API changes later.

Gábor Hojtsy’s picture

Issue tags: -DrupalWTF, -API clean-up, -D8MI, -langcode

#14: path-language-to-langcode-14.patch queued for re-testing.

Gábor Hojtsy’s picture

Issue tags: +DrupalWTF, +API clean-up, +D8MI, +langcode

#14: path-language-to-langcode-14.patch queued for re-testing.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
FileSize
30.06 KB

Patch did not apply anymore due to #1357918: Missing update for language_default in language langcode update landing. Minor change required to apply again. No other changes.

sun’s picture

As with #1357918-35: Missing update for language_default in language langcode update, I'd like to see a regular database update here.

Attached patch

1) moves the schema change into a regular system module update

2) tweaks drupal_lookup_path() to not look up any URL aliases in MAINTENANCE_MODE (which is long overdue).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Well, that maintenance mode chnange really has far reaching effects. Eg, if you put your site into maintenance mode, and your menu links would suddenly change. Let's do that in its own issue, not under the covers of language API changes unless it is explicitly related.

Gábor Hojtsy’s picture

In other words, the (db_table_exists('url_alias') && db_field_exists('url_alias', 'language')) condition in update_prepare_d8_bootstrap() would have done fine for a D8->D8 update just as well (it has a condition passing fine on existing D8 sites) without introducing side effects for when a site goes to maintenance mode to not use any path aliases. I think that is leaping to conclusions too fast there.

sun’s picture

if you put your site into maintenance mode

Note that MAINTENANCE_MODE != variable_get('maintenance_mode'). MAINTENANCE_MODE is only defined in install.php, update.php, and authorize.php.

Without this change, drupal_lookup_path() blows up with a PDOStatement exception right on the first update.php page, since drupal_path_initialize() as well as as any call to url() attempts to query {url_alias} for any url() being used in the update script, but the 'langcode' column does not exist yet. Thus, you don't even reach the stage of executing module database updates.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Yes, thats why it was in the bootstrap prepare update function, right? I think both methods are valid to some extent. Also both supply a D8 -> D8 upgrade path as explained above. But your solution has side effects (although not to the extent that I originally understood).

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Ok, well, either way works technically. Catch/Dries will choose from the two for their liking.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I committed Gabor's patch in #18 to 8.x because sun's patch in #19 may have unintended side effects (according to Gabor).

I'm marking this issue 'needs work' though, so we can continue to discuss the changes proposed by sun and follow-up if necessary.

Gábor Hojtsy’s picture

Added changelog at http://drupal.org/node/1413790.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Moving to fixed due to lack of discussion.

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