Please review this sandbox http://drupal.org/sandbox/vladimir-m/1861050 project.
Short description
This module allows us to sort the elements of a field which has the select list
widget.
Usage
For example:
We have a field called Months, which is a "List (text)" field with the "Select list" widget. In the "Allowed values list" for this field we have the following values:
January|January
February|February
March|March
April|April
May|May
June|June
July|July
August|August
September|September
October|October
November|November
December|December
In the "SORT OPTIONS
" fieldset:
- Check "Apply sort option
";
- Choose "Order by
" - order by text or by the selected value;
Explanation: If we will inspect (using FireBug or else) the Months element
from the form, we will have the following:
<select id="edit-field-months-und" name="field_months[und]"
class="form-select">
<option value="_none">- None -</option>
<option value="April">April</option>
<option value="August">August</option>
...
<option value="November">November</option>
<option value="October">October</option>
<option value="September">September</option>
</select>
Based on the code above, the "value" for an option will be "_none
"
and the text will be "- None -
".
- Choose "Sort
" - Ascending or Descending.
In our given form, we will be able to see the elements sorted by the chosen
criteria.
Furthermore, it is possible to add 2 new attributes for the custom created
forms: "#sort_order
" and "#order_by
".
"#sort_order
" - may contain values as "asc
" or "desc
" and
"#order_by
" may contain values as "text
" or "value
".
For example:
$form['selected_my_data'] = array(
'#type' => 'select',
'#title' => t('Selected Integer'),
'#options' => array(
'zero' => t('Zero'),
'one' => t('One'),
...
'seven' => t('Seven'),
'seven' => t('Six'),
'two' => t('Two'),
),
'#default_value' => 'nine',
'#order_by' => 'text', // May contain values as "text" or "value".
'#sort_order' => 'asc', // May contain values as "asc" or "desc".
);
As specified in the code above, the options for this element will be ordered by
text and ascendingly sorted.
For more details, please view this video: http://www.youtube.com/watch?v=7eYkUA28gvw
Sandbox project: http://drupal.org/sandbox/vladimir-m/1861050
The repository can be found here: git clone http://git.drupal.org/sandbox/vladimir-m/1861050.git select_option_sort
Module is for Drupal 7
Reviews of other projects
Stage I
http://drupal.org/node/1884388#comment-6922350
http://drupal.org/node/1883464#comment-6922562
http://drupal.org/node/1879654#comment-6923462
http://drupal.org/node/1860736#comment-6923564
Stage II
http://drupal.org/node/1905638#comment-7016012
http://drupal.org/node/1743318#comment-7016078
http://drupal.org/node/1887202#comment-7016128
http://drupal.org/node/1887890#comment-7016210
http://drupal.org/node/1899154#comment-7016228
Stage III
http://drupal.org/node/1911844#comment-7042650
http://drupal.org/node/1846814#comment-7042774
http://drupal.org/node/1905876#comment-7042904
Stage IV
http://drupal.org/node/1907552#comment-7044410
http://drupal.org/node/1867346#comment-7044614
http://drupal.org/node/1781984#comment-7044714
Thanks in advance for reviewing!
Comment | File | Size | Author |
---|---|---|---|
#11 | SelectOptionSort-SortableListFields-1884178-11.patch | 1.34 KB | davidmac |
#9 | Selection_001.png | 12.51 KB | vladimir-m |
#3 | Screen Shot 2013-01-09 at 11.14.40 AM.png | 274.71 KB | aboutblank |
#3 | Screen Shot 2013-01-09 at 11.21.12 AM.png | 263.74 KB | aboutblank |
Selection_012.png | 28.24 KB | vladimir-m |
Comments
Comment #1
DYdave CreditAttribution: DYdave commentedHi vladimir-m,
Thanks very much for submitting this great module.
Here is just a quick review from PAReview and it seems there are quite a few errors already:
1 - Master branch could be removed
2 - README.TXT: Problems: Do the files actually have extensions?
Obviously some coding standards errors reported in point 3.
3 - Coding standards:
An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
You can find the results of the automated report at LINK.
OR
I´ve attached the automated report as text file to this comment.
Code Review: http://ventral.org/pareview/httpgitdrupalorgsandboxvladimir-m1861050git
Hope this helps improving your module and that should get your validation started.
Feel free to let me know if you would have any questions or other comments on these items, I would be glad to help.
Cheers!
Comment #2
vladimir-m CreditAttribution: vladimir-m commentedHi DYdave,
Thanks very much for your quick response and useful reviews.
I just clean up the code. I made changes you mentioned and the ones related to coding standards.
Added README file and also removed master branch.
Thank you for your feedback.
Comment #3
aboutblank CreditAttribution: aboutblank commented1. I would recommend that in README.txt step 1 under INSTALLATION should clarify the sites/all/libraries installation directory structure, so it could read:
Download jQuery plugin from https://github.com/vladimir-m/jquery-select-option-sort to "sites/all/libraries" so that the directory structure is sites/all/libraries/jquery-select-option-sort/jquery.selectoptionsort.js
It seems obvious, but it could be a stumbling block for some and this seems to jive with installation structures elsewhere about adding libraries.
2. When it's sorting by alphabet, it it supposed to be case-sensitive? So for example, if the text fields are "Yelpo, Zapples, and xylophone," and I'm sorting Text Ascending, it shows up as:
with the lowercase 'x' word last. However, when I change the text fields to "Yelpo, Zapples, and Xylophone" and keep the same sorting settings, it's out put is
If this is expected behavior, maybe that makes sense, but it was a little confusing for me.
3. If I change the list widget to
Check boxes/radio buttons
, I can't actually make any selections on the node edit form. I've uploaded screenshots of what it looks like with this module disables and enabled including the firebug output.Comment #4
vladimir-m CreditAttribution: vladimir-m commentedHello aboutblank,
Thank you for detailed and useful review.
I made changes you mentioned in README.txt regarding Installation:
Also, I added some comments regarding
case sensitive
(2) andCheck boxes/radio buttons
widget type in README.txt andselect_option_sort_form()
(3).Thanks.
Comment #4.0
vladimir-m CreditAttribution: vladimir-m commentedAdde Reviews of other projects section.
Comment #5
monymirzaselect_option_sort.module
line #69 - spell mistake - 'Inorder'
Comment #6
vladimir-m CreditAttribution: vladimir-m commentedHello monymirza,
Thank you for feedback.
Spell mistake was fixed.
Thank you!
Comment #6.0
vladimir-m CreditAttribution: vladimir-m commentedAdded Reviews of other projects section.
Comment #7
vladimir-m CreditAttribution: vladimir-m commentedreview bonus
Comment #7.0
vladimir-m CreditAttribution: vladimir-m commentedAdded new item in Reviews of other projects section.
Comment #7.1
vladimir-m CreditAttribution: vladimir-m commentedAdded formatation to description.
Comment #8
klausimanual review:
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #8.0
klausiAdded item to Reviews of other projects section.
Comment #8.1
vladimir-m CreditAttribution: vladimir-m commentedreview
Comment #9
vladimir-m CreditAttribution: vladimir-m commentedHello,
Module Select option sort was reformatted.
Now Select option sort module does not use third library and was added an extensive description.
Please review this project.
Thank you.
Comment #9.0
vladimir-m CreditAttribution: vladimir-m commentedreview
Comment #10
vladimir-m CreditAttribution: vladimir-m commentedAdding tag "PAReview: review bonus"
See "Stage II" in issue summary,
Please review this project.
Thank you in advice.
Comment #11
davidmac CreditAttribution: davidmac commentedWell done on creating a nice, handy module. I've installed it and looked through the files. I initially have some points on basic housekeping if you don't mind.
The text on the help page would benefit from being tidied up, including its length and layout. The objective here is to provide help that is applicable to all users, both new and advanced. Should this help page really include arrays and code?, I would advise confining any developer specific help to the README.txt file.
In addition, the help page for this module does not refer to Fields, CCK or Field UI, all of which are of importance in this modules context.
Given that the module name doesn't indicate that it relates to 'Fields' you may want to change the package to 'Fields' in your modules' .info file. To find this module under the Fields package in Modules Admin would be more intuitive for the end user as it is more relevant, and is used by other modules as a category.
This module doesn't specify any dependencies. If enabled in its current form, you cannot use it from the interface, without enabling Fields UI. I would suggest that its good practice to make the dependicies known and necessary, therefore I've amended the .info file accordingly.
I've made some suggestions in the attached patch including:
README.txt - 'ascendingly' is not a word, - changed to 'sorted in ascending order'
README.txt - Inserted 'DEPENDENCIES' section to explain relationship to CCK and 'Fields UI'
select_option_sort.info - added 'dependencies[] = field_ui'
select_option_sort.info - suggest changing 'package' to 'package = Fields' eventually.
Comment #12
darrenwh CreditAttribution: darrenwh commentedsetting as needs work as per above comments
Comment #13
heddnREADME feedback
select_option_sort.admin.inc feedback
if (!empty($results))
could be writen simplyif ($results)
as the interface offetchAssoc()
in_select_option_sort_check_field_exist()
returns a result orFALSE
. The same applies in select_option_sort.module on line 279, etc. so look around for other times you call_select_option_sort_check_field_exist()
.<pre>
tag is what you want?select_option_sort.install
hook_uninstall
is completely unnecessary. Those variables aren't used anywhere in the current code base.select_option_sort.module
@file
description is incorrect given the current implementation of this moduleif (is_array($rows))
is unnecessary._select_option_sort_get_database_records
declares an array or false as its signature. Perhaps useif ($rows)
_select_option_sort_sort_elements
- Nested if statements would make more sense. example: if text, then nested if asc/desc followed by elseif asc & else._select_option_sort_check_field_exist
- It's my personal peeve to only see a single return in a function. Unless you are dealing with a guard clause. Try assigning the return to a variable and structure your code so that value gets returned on the final line of the function._select_option_sort_get_database_records
- while loop on$query_results
is uncessary. UsefetchAll()
instead.Comment #14
vladimir-m CreditAttribution: vladimir-m commentedThank you very much for review.
I made the changes mentioned. Now module is done for next review.
Comment #15
davidmac CreditAttribution: davidmac commentedVladimir,
I understand that the constant alternation between NR and NW is frustrating. However, I still think the help page is not very explanatory for a novice user. Instead of simply making the changes pointed out by reviewers, can I suggest that you 'rethink entirely' the help page. Is there any need for the incomplete example and the dropdown listbox? (you show the before but not after). Perhaps a more simplified approach and something for the novice user such as "Choose a node type such as an Article, and go to manage fields under
/admin/structure/types/manage/article/fields
etc., etc.". New users take a while to discover the README.txt file, that's why the help page is relevant.This is simply a suggestion, the help page isn't critical, and I've seen worse elsewhere, so leaving it at NR.
Comment #16
yan CreditAttribution: yan commentedI tried the module and it does what I expected it to do from the description (sort the content of a list alphabetically).
I think the help text and the readme file are still kind of confusing. It would be nice to have an easier text with less code. And isn't the example with months a bad one? At least months are an example where you usually wouldn't sort alphabetically. Countries might be better to show the use.
When I change the sort order and the option "none" is sent to the bottom, the first value of the select list is selected by default. Shouldn't it still be "none"?
Some additional comments (just small things or suggestions):
select_option_sort.module, line 193:
Should be "field exists" (with an "s").
select_option_sort.module, line 208 and 230:
I didn't get those errors on install, but I guess they'd confuse me: Shouldn't the module make sure that everything is ok? If on install I am told to reinstall, that's kind of strange.
Also, "details can be found in the select_option_sort/README.txt file" is not very clear. Maybe it would be better to link to the module's help page?
select_option_sort.install, line 21, 28, 25:
Three fields have the same description. Is that intended?
README.txt, lines 61 and 62:
Maybe that part could go to a section "for developers"? At least for me it's not clear what is meant by "for the custom created forms"
Comment #17
Nikro CreditAttribution: Nikro commentedHi vladimir-m,
It seems to be a very useful module! Great work.
I've tested it with fieldgroup module and it does work as expected.
select_option_sort.module
Instead of using
template_preprocess($variables, $hook)
, I recommend you to usetemplate_preprocess_hook($variables)
because anyway you are hooking only into page preprocess.select_option_sort_form_alter(&$form, &$form_state, $form_id)
This will be fired for all the existing forms (because you don't use $form_id). And it also makes db queries each time. Also I'd recommend you to use
select_option_sort_permission
You declare them but never use them. Either delete the declaration or use it somewhere.
So far that's all I found.
Great documentation / README.txt file.
Also I guess your sorting won't handle Node Edit forms within Panels, maybe that's something interesting to work on for the future.
Thanks!
Comment #17.0
Nikro CreditAttribution: Nikro commentedReviews of other projects
Comment #18
vladimir-m CreditAttribution: vladimir-m commentedThank you Yan and Nikro for review.
I made the changes mentioned.
Nikro, regarding select_option_sort_form_alter().
I had to do a form alter without a form id because there may be custom elements written in the source files which do not belong to a form with a specified id.
Now module is done for next review.
Thank you very much!
Comment #19
davidmac CreditAttribution: davidmac commentedSome documentation corrections:
select_option_sort.admin.inc - line 63 change "order to sort" to "Sort order" in 2 instances.
select_option_sort.module - line 101 is not clear, suggest "Assign #order_by and #sort_order paramaters to form elements".
select_option_sort.module - line 176 insert a space between "function." and "Save settings".
select_option_sort.module - line 183 "update records (data) into the database" change "into" to "in".
Although you've gone to the extra effort of attaching a video link to the help page(albeit without any audio), for a module of this size, the instructions could easily be (and should be) self contained within the module, without relying on external resources, which may not always be available. In cases where help documentation is very lengthy, such as the "Rules" module, help documentation is hosted on Drupal.org.
Comment #20
yareckon CreditAttribution: yareckon commentedHi vladimir-m,
A couple of issues,
It seems like your module does not handle multilingual fields well. When I enable the i18n module suite and translate the field options, then I not only don't get any sorting, but get some errors as well.
This is because you are testing for language und, then testing for global language as a fallback, then giving up. You don't need to do this because at least in forms, fields have a #language property. Read it and look for the options in that language. If this module is intended to sort stuff in the front end you may not be able to do that and will have to check for options in content language or system language.
Secondly, I'm a little confused why this is a page preprocessor... I guess so that you can influence stuff in the front end and the backend? All of your documentation, and your youtube demo all show this as a form focused module, however, so that's how I'm taking it.
Because you are just manipulating $variables['page']['content']['system_main'], I'm worried that people who put forms in other areas of the page might really be surprised to see them suddenly unsorted. On the other hand, looping through every area on the page looking for your fields might be pretty exhaustive and unperformant. So what to do? Maybe embrace that you are working on a backend module that deals with forms and focus on a hook form alter implementation rather than processing the page array at render time?
I guess this is just an invitation for discussion and clarification, because these use cases are not that unlikely. Let's figure this out.
Comment #21
vladimir-m CreditAttribution: vladimir-m commentedThank you David Mc Donnell and yareckon for review!
yareckon , regarding multilingual I double check in different Drupal instance with different languages (ro, ru, fr) and all looks fine, maybe it's something specific to your environment set?
I also made some improvements. Now module is done for next review.
Thank you very much!
Comment #22
yareckon CreditAttribution: yareckon commentedMy review was done is using the Entity Translation module, that exposes a GUI for working with drupal's built-in per field translations.
That means a single node has multiple language versions, and only some fields are translatable. This is drupal 7 core stuff, and will be even more visible in drupal 8.
What can result is that I have multiple languages on a field. At render time in $variables a language can be used that is not 'und' or global $language->language. The classic case for this is the node edit form in a language other than the default language on that node. You should just look at the field's #language property to see what language you want if you are doing this in a $variable preprocess function.
The other part of my environment that is non standard was the i18n field settings translation module, where you can indeed translate the actual select item text. That doesn't have anything to do with the errors above, but was an additional part of the way I tested your module if you're interested in my environment. Most of that functionality is going to be used on many multilingual sites as well.
What do you think about my other questions about whether it is appropriate to be doing this in a preprocess function versus direct form manipulation? Do you want to respond to them? What happens if the form is in the sidebar? Right now the select fields will not be sorted in that case.
Comment #23
vladimir-m CreditAttribution: vladimir-m commentedHello yareckon,
Thank you very much for review.
Finally I've managed to reproduce this bug. It was already fixed and works fine even with multilingual. Taking into consideration your suggestion, the process part was moved to form_alter.
I advise you to update the module (git pull) and again thank you for the review.
Now module is done for next review.
Thank you very much!
Comment #24
yareckon CreditAttribution: yareckon commentedvladimir-m, thank you this looks much better. The errors are now gone, and sorting works in each language. Good work!
The one thing that remains is the no value option '- None -'. It is being sorted too! So it's almost like changing the default option! This is easiest to see on a new node with no value in the field. If you sort ascending, it will be - None - shown default first, then the other options follow (unless you have an option starting with an ascii value less than "-"). On the other hand, if you sort descending, None is no longer the default, but lands far down the list. Could you take out the "none" option, then sort, and then put it back in? Make sure that the user's desired default value doesn't change despite your sorting!
Thanks! This is getting there!
Ryan
Comment #25
vladimir-m CreditAttribution: vladimir-m commentedHi yareckon,
Thank you very much for review and suggestion.
Issue with '- None -' value was fixed. I advise you to update the module (git pull) and again thank you for the review.
Now module is done for next review.
Thanks!
Comment #26
davidmac CreditAttribution: davidmac commentedSome behaviours for you to look into:
After adding a "select list" to a page type, setting sort order to descending, then selecting a few months in a page with this field, the months were in descending order as expected. I then editied the field and changed the sort order to ascending, flushed caches and ran cron, but the months remained in descending order. The only way to initiate the new ascending order was to edit and re-save the page in question. This is not what you would expect, particulary if using multiple nodes with this field sorting option.
I know from comments above that you removed the permissions hook as it was not implemented, however testing this as an 'Authenticated User' with "Administer Content Types" permission, results in the situation where I can edit and add fields such as selection lists, but CANNOT set the sort order, i.e. sort order options are denied under the field edit form. Its reasonable to expect a user with the ability to edit and add fields to be able to administer your modules' settings also, so perhaps looking at access arguments is needed.
Comment #27
vladimir-m CreditAttribution: vladimir-m commentedHi David Mc Donnell,
Thank you for review.
1. I double check in different Drupal7 environment:
- create a Select field contained months;
- sort value DESC
- save;
- edit field;
- change sort option to ASC;
- save;
- clear cache;
- run cron manually;
All looks fine, maybe it's something specific to your environment set? Please pull the updated code and re-install module.
2. I added "administer select option sort field settings" permission.
Now module is done for next review.
Thanks!
Comment #28
davidmac CreditAttribution: davidmac commentedHi Vladimir,
Based on your updated module repo, the issue is unchanged.
Looking at step 1 in your comment above, you don't mention examining field order on a particular page that uses this sorting option, so I wonder if we are talking about the same thing.
Looking at my Drupal environment, it is a simple production environment with caching of nodes for anonymous visitors off. I've tested it across different fresh installations. The problem is that, in this case, the 'cache_fields' table is only updated on re-saving a node, not when the sort order is changed. Examining the 'cache-fields' table for say 'node/2' after changing sort order shows the field in its previous order.
Are you testing in a production or development environment?
Comment #29
vladimir-m CreditAttribution: vladimir-m commentedHello David Mc Donnell,
Thank you for review and testing.
Issue with Select option sort on node view was fixed, please pull the updated code.
Now module is done for next review.
Thanks!
Comment #30
klausimanual review:
Using t() and watchdog() correctly is an important skill to avoid XSS vulnerabilities in your code (although I could not see a security hole here). Otherwise looks nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #30.0
klausivideo added.
Comment #31
vladimir-m CreditAttribution: vladimir-m commentedHello klausi,
Thank you in advice for review.
I just finish all rectifications that you've mentioned.
Adding tag "PAReview: review bonus"
See "Stage III" in issue summary.
Kind regards
Vladimir
Comment #32
davidmac CreditAttribution: davidmac commentedHi Vladimir,
It would be helpful to the review process, and for others who may read this, if you would be kind enough to explain briefly, the changes you have made between #27 - #29 above.
Thanks,
Comment #33
klausimanual review:
But otherwise looks RTBC to me now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #34
vladimir-m CreditAttribution: vladimir-m commentedHi klausi,
Thank you for review.
- In order_by field description changed t() function @placeholder.
Comment #35
vladimir-m CreditAttribution: vladimir-m commentedHello David Mc Donnell,
After you suggest to look in to 'cache-fields' direction, I start to investigate the issue regarding Select option sort on node view, and I found what select field default options order depend on field delta that is stored in field_data_field_[field_name] table and as solution I use hook_node_view_alter() to reorder default options.
Thank you for your efforts in promoting this module.
Comment #36
davidmac CreditAttribution: davidmac commentedThanks Vladimir,
Glad to see its now ready, I look forward to using your module.
Comment #36.0
davidmac CreditAttribution: davidmac commentedAdded "Stage III" section to "Reviews of other projects" section.
Comment #37
vladimir-m CreditAttribution: vladimir-m commentedReview bonus added.
See Stage IV in Reviews of other projects section.
Comment #38
klausino objections for more than a week, so ...
Thanks for your contribution, vladimir-m!
I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #39.0
(not verified) CreditAttribution: commentedReviews of other projects. Stage IV.