Comments

rodrigoaguilera’s picture

Status: Active » Needs review
StatusFileSize
new8.17 KB

Here's the patch.

Is ready to do it for every bundle of every entity but only exposes the interface for nodes in the content type edit form. You can set the variable manually if you are interested in other entities.

$default = variable_get('pathauto_i18n_default_' . $type . '_' . $bundle, 1);

It gets a variable to store the default.

Sometimes the default was 1 and sometimes was 0 so I unified it to what I think is the sensible default, 1. Enable the module and you get the behavior.

Status: Needs review » Needs work

The last submitted patch, 1: 2463109-default-per-content-type-1.patch, failed testing.

rodrigoaguilera’s picture

StatusFileSize
new695 bytes

I'm not able to run the tests for this module on simplytestme or my local machine. I'm posting a void patch to check if everything is green.

rodrigoaguilera’s picture

Status: Needs work » Needs review
rodrigoaguilera’s picture

StatusFileSize
new8.79 KB
new644 bytes

Still not able to run the test properly but looking at the test I think I know what happens: with the default set to 1 now it generates more aliases for nodes and the users aliases en up in the second page of the aliases list so I changed the test to be lees weak and filter by user.

rodrigoaguilera’s picture

Status: Needs review » Needs work
rodrigoaguilera’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2463109-default-per-content-type-5.patch, failed testing.

rodrigoaguilera’s picture

Is not that.
Setting the default behavior for user entities to be 1 is the problem (inside pathauto_i18n_init_property). In this case the default language is trimmed from the prefix of the alias. in the tests ends up being "users/pathautoi18n" instead of "en/users/pathautoi18n" For the English language.

This only happens in the tests which is confusing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
StatusFileSize
new9.29 KB
new1.01 KB

Now I can run the test properly on my local machine altough you have to launche theme several times in simplytestme to get them green.

I found out that setting a prefix in an alias is a bad idea since if you are navigating the site in a different language other than the default you will get a prefix set by drupal. and if drupal finds that something is in the default language removes the prefix (the test fail).

I modified the test so it tests the same functionality but without the language prefixes (now they are suffixes).

Status: Needs review » Needs work

The last submitted patch, 10: 2463109-default-per-content-type-10.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
StatusFileSize
new8.88 KB
new488 bytes

Forget about the prefix gettting trimmed, it was getting the very initial pathauto default "users/[user:name]".
There was some static cache getting in the middle.

ciss’s picture

Status: Needs review » Needs work

@rodrigoaguilera: You're changing the original default (false) in several places to true. I don't think that's a good idea. The patch should only provide the ability to set a default per entity type, not change the original behavior.
Other than that, the patch looks good to me.

rodrigoaguilera’s picture

@ciss
Great!
As I said

Sometimes the default was 1 and sometimes was 0 so I unified it to what I think is the sensible default, 1. Enable the module and you get the behavior.

I feel that the UX is not consistent because the option for creating aliases for all languages is checked by default when internally the value is 'unchecked' because you haven't made a choice yet.

For example line 70 http://cgit.drupalcode.org/pathauto_i18n/tree/pathauto_i18n.module#n70
and
http://cgit.drupalcode.org/pathauto_i18n/tree/modules/pathauto_i18n_node...

This causes that if you use the module without interface (creating nodes programmatically for example) you get a different behavior than clicking around.(i18n paths created or not).

For me the sensible default is TRUE but I don't really care.

Can you comment on what you think should be the default?

ciss’s picture

@rodrigoaguilera: I agree with every point you make, but for this patch we shoul leave the defaults as they are, since this issue is about adding a new feature.
Normalizing the defaults should be a separate issue, in my opinion.

Edit: As for the actual value, I think the default should be FALSE, to not break or interfere with any other modules.

ciss’s picture

Status: Needs work » Needs review
StatusFileSize
new9.46 KB
new2.56 KB

Changes: Set the defaults back according to #13, add getter and setter functions for defaults.

ciss’s picture

ciss’s picture

Another thing we have to consider is how to handle overrides. Currently pathauto_i18n immediately stores a setting for each entity it encounters. We should probably skip that save if it matches the default.

sanchiz’s picture

StatusFileSize
new9.49 KB
new880 bytes

Hi guys!

The original behaviour of this module - Pathauto i18n is enabled for all content types. This means that we should keep this behaviour. When I've applied patch from comment #16, on content type edit form I see FALSE, but on node dfit form I see TRUE.

I've slightly updated patch to apply default value in all places in order to keep default behaviour.

See interdiff.

  • sanchiz committed 9484b07 on 7.x-1.x authored by rodrigoaguilera
    Issue #2463109 by rodrigoaguilera, ciss, sanchiz: Ability to set the...
  • sanchiz committed b20cf27 on 7.x-1.x authored by ciss
    Issue #2463109 by rodrigoaguilera, ciss, sanchiz: Ability to set the...
sanchiz’s picture

Status: Needs review » Fixed

Thank you guys! Committed.

Available now in 7.x-1.x-dev or in in 7.x-1.4 and later once released.

sanchiz’s picture

Issue tags: +CodeSprintUA, +dcuacs2015

Status: Fixed » Closed (fixed)

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