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!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DYdave’s picture

Status: Needs review » Needs work

Hi 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

FILE: /var/www/drupal-7-pareview/pareview_temp/js/select_option_sort.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
9 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/select_option_sort.admin.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | The second line in the file doc comment must be " * @file"
4 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
10 | ERROR | Missing comment for @return statement
31 | ERROR | Array indentation error, expected 4 spaces but found 6
46 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/select_option_sort.install
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | The second line in the file doc comment must be " * @file"
4 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
10 | ERROR | Inline comments must start with a capital letter
10 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
14 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/select_option_sort.module
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AND 2 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
2 | WARNING | There must be no blank line following an inline comment
2 | ERROR | You must use "/**" style comments for a file comment
5 | WARNING | Format should be "* Implements hook_foo()." or "Implements
| | hook_foo_BAR_ID_bar() for xyz_bar()."
5 | ERROR | Function comment short description must end with a full stop
8 | ERROR | Inline comments must start with a capital letter
8 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
20 | ERROR | Inline comments must start with a capital letter
43 | ERROR | Do not use t() in hook_menu()
--------------------------------------------------------------------------------

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!

vladimir-m’s picture

Status: Needs work » Needs review

Hi 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.

aboutblank’s picture

Status: Needs review » Needs work
FileSize
263.74 KB
274.71 KB

1. 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:

Yelpo
Zapples
xylophone

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

Xylophone
Yelpo
Zapples

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.

vladimir-m’s picture

Status: Needs work » Needs review

Hello aboutblank,
Thank you for detailed and useful review.
I made changes you mentioned in README.txt regarding Installation:

1. 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";

Also, I added some comments regarding case sensitive (2) and Check boxes/radio buttons widget type in README.txt and select_option_sort_form() (3).

Thanks.

vladimir-m’s picture

Issue summary: View changes

Adde Reviews of other projects section.

monymirza’s picture

select_option_sort.module
line #69 - spell mistake - 'Inorder'

vladimir-m’s picture

Hello monymirza,
Thank you for feedback.

Spell mistake was fixed.

Thank you!

vladimir-m’s picture

Issue summary: View changes

Added Reviews of other projects section.

vladimir-m’s picture

Issue tags: +PAreview: review bonus

review bonus

vladimir-m’s picture

Issue summary: View changes

Added new item in Reviews of other projects section.

vladimir-m’s picture

Issue summary: View changes

Added formatation to description.

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

manual review:

  1. What is the purpose of the module? What problem does it solve? Please add a use case to the module page and/or the README.txt. I took me 10 minutes to realize that it just sorts select option elements alphabetically, right?
  2. select_option_sort_init(): why do you add your javascript on every page? Shouldn't it be added only on form elements, i.e. through hook_form_alter() and #attached? And if it only applies to select lists anyway you should implement the appropriate preprocess hook for that element and add the javscript there. Then the JS is only added if there is an actual select list on the page.
  3. Then I realized: why would you even sort in Javascript? Just rearrange the elements in a preprocess hook in PHP, then you get increased front end performance because you don't have to load and execute that Javascript at all.
  4. select_option_sort.js: indentation errors, always use 2 spaces per indentation level.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

Issue summary: View changes

Added item to Reviews of other projects section.

vladimir-m’s picture

Issue summary: View changes

review

vladimir-m’s picture

Status: Needs work » Needs review
FileSize
12.51 KB

Hello,
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.

vladimir-m’s picture

Issue summary: View changes

review

vladimir-m’s picture

Issue tags: +PAreview: review bonus

Adding tag "PAReview: review bonus"

See "Stage II" in issue summary,
Please review this project.
Thank you in advice.

davidmac’s picture

Well 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.

darrenwh’s picture

Status: Needs review » Needs work

setting as needs work as per above comments

heddn’s picture

README feedback

  • The #order_by FAPI option, 'text' could be improved upon. Label is an option I've commonly seen used. The reason it doesn't make sense is when I use numerical values for days of the month, etc. In that case, it isn't text, its numbers. This would cause some rewrite so you might consider other's input before you make this change.
  • The installation instructions could reference the standard D7 install instruction page (https://drupal.org/documentation/install/modules-themes/modules-7) as there isn't anything special.

select_option_sort.admin.inc feedback

  • line 17: if (!empty($results)) could be writen simply if ($results) as the interface of fetchAssoc() in _select_option_sort_check_field_exist() returns a result or FALSE. 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().
  • line 51: check_plain is unnecessary I think. Perhaps <pre> tag is what you want?

select_option_sort.install

  • The 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 module
  • if (is_array($rows)) is unnecessary. _select_option_sort_get_database_records declares an array or false as its signature. Perhaps use if ($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. Use fetchAll() instead.
vladimir-m’s picture

Status: Needs work » Needs review

Thank you very much for review.
I made ​​the changes mentioned. Now module is done for next review.

davidmac’s picture

Vladimir,

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.

yan’s picture

Status: Needs review » Needs work

I 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:

* Helper function. Check if field exist in select_option_sort table.

Should be "field exists" (with an "s").

select_option_sort.module, line 208 and 230:

drupal_set_message(t("An error occurred while installing the module. The select_option_sort table couldn't be found. We would recommend you to reinstall the Select option sort module. The needed details can be found in the select_option_sort/README.txt file."), 'error');

drupal_set_message(t("An error occurred while installing the module. The select_option_sort table couldn't be found. We would recommend you to reinstall the Select option sort module."), 'error');

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:

'description' => 'The {node_type} of this node.',

Three fields have the same description. Is that intended?

README.txt, lines 61 and 62:

Furthermore, it is possible to add 2 new attributes for the custom created
forms: "#sort_order" and "#order_by".

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"

Nikro’s picture

Hi 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 use template_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!

Nikro’s picture

Issue summary: View changes

Reviews of other projects

vladimir-m’s picture

Status: Needs work » Needs review

Thank 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!

davidmac’s picture

Status: Needs review » Needs work

Some 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.

yareckon’s picture

Hi 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.

    Notice: Undefined index: #options in select_option_sort_preprocess_page() (line 81 of ...select_option_sort/select_option_sort.module).
    Warning: natcasesort() expects parameter 1 to be array, null given in _select_option_sort_sort_elements() (line 120 of ...select_option_sort/select_option_sort.module).
    Warning: array_reverse() expects parameter 1 to be array, null given in _select_option_sort_sort_elements() (line 121 of ...select_option_sort/select_option_sort.module).

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.

vladimir-m’s picture

Status: Needs work » Needs review

Thank 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!

yareckon’s picture

Status: Needs review » Needs work

My 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.

vladimir-m’s picture

Status: Needs work » Needs review

Hello 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!

yareckon’s picture

Status: Needs review » Needs work

vladimir-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

vladimir-m’s picture

Status: Needs work » Needs review

Hi 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!

davidmac’s picture

Status: Needs review » Needs work

Some 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.

vladimir-m’s picture

Status: Needs work » Needs review

Hi 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!

davidmac’s picture

Status: Needs review » Needs work

Hi 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?

vladimir-m’s picture

Status: Needs work » Needs review

Hello 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!

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

manual review:

  1. Git commit mesages: " by vladimir-m: Module improvement.": does not really say anything, see http://drupal.org//node/52287
  2. select_option_sort_install(): do not juggle with module weights, this is very unreliadble. Use hook_module_implements_alter() if you want to run before/after specific hook implementations.
  3. _select_option_sort_get_database_records(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
  4. _select_option_sort_change_db_row(): do not concatenate variables into watchdog() messages, use placeholders instead.
  5. _select_option_sort_change_db_row(): use drupal_write_record() instead of db_insert()/db_update and you get schema validation for free.
  6. _select_option_sort_field_settings_form_alter(): do not concatenate translatable strings for your #description, use placeholders with t() instead.

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.

klausi’s picture

Issue summary: View changes

video added.

vladimir-m’s picture

Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Hello 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

davidmac’s picture

Hi 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,

klausi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  1. "'!options' => check_plain('Moldova'),": use the @placeholder and it will automatically run check_plain() for you. See the documentation of t().

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.

vladimir-m’s picture

Issue tags: +PAreview: review bonus

Hi klausi,
Thank you for review.
- In order_by field description changed t() function @placeholder.

vladimir-m’s picture

Hello 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.

davidmac’s picture

Thanks Vladimir,

Glad to see its now ready, I look forward to using your module.

davidmac’s picture

Issue summary: View changes

Added "Stage III" section to "Reviews of other projects" section.

vladimir-m’s picture

Review bonus added.
See Stage IV in Reviews of other projects section.

klausi’s picture

Status: Reviewed & tested by the community » Fixed

no 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.

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

Anonymous’s picture

Issue summary: View changes

Reviews of other projects. Stage IV.