The initial value of the checkbox is hardcoded to be set to FALSE. provide a variable to set the value per content type
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | interdiff.txt | 880 bytes | sanchiz |
| #19 | pathauto_i18n-node_type_default-2463109-19.patch | 9.49 KB | sanchiz |
| #16 | pathauto_i18n-node_type_default-2463109-16.patch | 9.46 KB | ciss |
| #16 | interdiff.txt | 2.56 KB | ciss |
Comments
Comment #1
rodrigoaguileraHere'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.
Comment #3
rodrigoaguileraI'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.
Comment #4
rodrigoaguileraComment #5
rodrigoaguileraStill 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.
Comment #6
rodrigoaguileraComment #7
rodrigoaguileraComment #9
rodrigoaguileraIs 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.
Comment #10
rodrigoaguileraNow 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).
Comment #12
rodrigoaguileraForget 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.
Comment #13
ciss commented@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.
Comment #14
rodrigoaguilera@ciss
Great!
As I said
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?
Comment #15
ciss commented@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.
Comment #16
ciss commentedChanges: Set the defaults back according to #13, add getter and setter functions for defaults.
Comment #17
ciss commentedComment #18
ciss commentedAnother 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.
Comment #19
sanchiz commentedHi guys!
The original behaviour of this module -
Pathauto i18nis 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 seeFALSE, but on node dfit form I seeTRUE.I've slightly updated patch to apply default value in all places in order to keep default behaviour.
See interdiff.
Comment #21
sanchiz commentedThank you guys! Committed.
Available now in 7.x-1.x-dev or in in 7.x-1.4 and later once released.
Comment #22
sanchiz commented