Vehicle

Came up with some minor warnings and errors using coder (http://drupal.org/project/coder). See report.

Hi Ignigena,

Since vehicle.module doesn't do anything besides implement hook_menu() (for the admin/vehicle) path, You may want to consider merging the vehicle and vehicle_ymm modules.

The primary reason is to smooth the installation process. The installation instructions in the README refer to vehicle as a single module ("Enable the module and configure settings") and when I installed via Drush, I didn't realize there are actually four modules to be enabled (there are no dependencies to listed in vehicle.module).

Plus, the content presented at /admin/vehicle just said that I "have no administrative items" (not that I need to enable other modules).

I ran Coder reviews on all four modules included, and the reports came back clean.

However, I hit problems while trying to add vehicle information though: I'm unable to create a Make. I receive an error that "The machine-readable name is already in use. It must be unique." (I've attached a screenshot)

When I try customizing the machine name, the field description says to use underscores, but doing so results in a validation error that says to use hyphens. When I used hyphens, I received the uniqueness error above (I receive that same error if I remove all hyphens and underscores.

Also, in vehicle_ymm.css, consider using a CSS selector that limits your the custom styling to module-specific items rather than form.select, which could potentially interfere with non-Vehicle styles.

Let me know if there's a setting or other dependency I may be missing. I'll retest then and let you know how it goes.

Thanks for your reviews, just make sure that you pick the oldest applications first.

Review of the 7.x-1.x branch:

• Remove "version" from the ./vehicle_model/vehicle_model.info file, it will be added by drupal.org packaging automatically.
• Remove "version" from the ./vehicle_make/vehicle_make.info file, it will be added by drupal.org packaging automatically.
• Remove "version" from the ./vehicle_ymm/vehicle_ymm.info file, it will be added by drupal.org packaging automatically.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. vehicle_make_list_entities(): this is vulnerable to XSS exploits, as you are printing the country unsanitized which is user provided input. If I enter <script>alert('XSS');</script> as country I get a nasty Javascript popup. Please read http://drupal.org/node/28984 again. Same in vehicle_make_vehicle_page(). Please check all your code where you print user provided text.
2. vehicle_make_pathauto_bulkupdate(): why do you need the extra select query? Just use entity_load() with FALSE as $ids?
3. "$result->rowCount() . ' vehicle makes were updated.'": all user facing text must run through t() for translation, and you should use placeholders with t() for the dynamic number.

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

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi,
This looks like a promising module, and I am currently after a module like this for a car parts site I am building.

I have just installed this module on a clean drupal install and will give you my impressions as I went through the process of creating a vehicle part on the site :)

• After installing all the vehicle modules I liked that the vehicle menu was at the top level and easy to find.
• I created 2 vehicles, both a make and model Holden Commodore, 1 sedan and 1 wagon (bodytype field).
• When creating the model, I was surprised when I saw there were many fields (engine position, seats etc.). This is a fieldable entity, why are these fields already on the entity? I understand why a model reference is needed, and maybe the year but I don't understand why the other fields are there.
• I created a part content type and added a vehicle reference field.
• I went to create a part thinking that I would be able to assign the part to a holden commodore sedan and then another part to the wagon but the vehicle reference field had year, make model and trim comboboxes. I hadn't entered a value for the trim on the vehicles and couldn't select the bodytype at all, the trim combobox had 2 values both on 'none'. This is a little confusing and maybe a normal entity reference field would have been better for me on the part content type.

I'm not sure if any of this will be helpful to you, but I thought you might like the feedback from someone who was looking at actually using this module on a production site. The functionality seems to be a little too specific for my needs so I don't think I will use it, but it does give me a head start in creating my own module.
Thanks

Hello Ignigena,

This looks like a promising module for anyone working with car parts.

However, when I took your module for a test drive, I found some errors.

After adding one or two make records, the application started to return 500 errors whenever I tried to add or edit another make record. This started after I tried to edit the machine name for a make record. The records were being added or updated correctly in the database, but the module would still return a 500 error.

I was able to add model records successfully, but I would suggest that you provide the user with some kind of confirmation message after a record has been added. Without any kind of feedback from your module, after adding a record I was left wondering if the add worked or not.

I added the Vehicle Make/Model Selector block to my site and it worked nicely. However, if I tried to select a vehicle for which there were no matching records, the application displayed a php error. The same thing happened when I selected Vehicle Makes from the Navigation menu and then picked a particular model that didn't have any matching records.

Finally, I see that there are a lot of warning messages being logged when I run your module.

Best of luck with your continuing work on this module.

Bob

Hi, Just curious..why having the focus on vehicle, and not on an entity which could be a vehicle?
This way the module can be more generic applicable?
Greetings, Martijn

Hi Ignigena,

Here's a more detailed explanation of the errors I receive when adding Make records.

When I first login, I can add new Make records without any problems.

However, when I try to edit the machine name while adding a record, I get the error message below:

The module did add the Make record I was trying to add, but it returned this error.

Once this problem has occurred, I continue to receive an error message whenever I try to add or edit any Make record.

The error condition goes away when I logoff and then log back on.

All my testing was done on a clean install of D7.

Hope this helps.

Bob

Hiya, Installed your module. It seems to be just what i need if i redevlope my website in drupal.

I seem to have found a bug, i cannot get Vehicle field to save correctly. Only the 'Make' saves the rest are not. Drupal throws an error of "An illegal choice has been detected. Please contact the site administrator."

Drupal Logs show "Illegal choice 2001-2002 in field_veh element."

Hi, I was under the impression it was for cars, but with your explanation it is generic enough I think. You are right that being to generic can loose functionality.
Sorry for my remark. Greetings, Martijn

Thanks for taking the time to develop this module! I sell car parts from my site, and it seems designed to provide exactly the kind of functionality that I need to make it easier for customers to find what they need. I've installed it (no problems), configured it with two vehicle makes and models (no problems), and started doing a little testing. I've found a few small behaviors that aren't quite what I expect. I'm using Ubercart for my shopping cart.

I created a Ford make and Mustang model with start model year 1967 and end model year 1998. I edited my Ubercart product content type and added a make/model field (Year/Make/Model dropdown widget) with no restriction on the number of make/model fields.

I have a part that fits 1967 - 1969 Ford Mustangs. I edit the product that corresponds to the part and add a field for a 1967 Ford Mustang, and 1968 Ford Mustang, and a 1969 Ford Mustang. Save the changes. When I edit the product again I see all three fields, but they all say "1967 Ford Mustang". I would expect to see one for each model year. Before and after images attached

Note the extra unused widget in the images. It would be nice to be able to remove it if I later want to remove a particular year/make/model.

I also tried the Year range model dropdown, but that only allows me to specify the complete range of years (1967 - 1998 in this case). It would be nice if I could just select 1967 - 1969.

Hi ,

There is still some coding standered issue.

Link : http://ventral.org/pareview/httpgitdrupalorgsandboxignigena1650668git

FILE: ...ar/www/drupal-7-pareview/pareview_temp/vehicle_make/vehicle_make.module
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
52 | ERROR | Invalid @param data type, expected int but found integer
54 | ERROR | Invalid @param data type, expected bool but found boolean
487 | ERROR | else must start on a new line

Thanks for the feedback. I was thinking that I could get the behavior I want by creating a separate make/model for each year. I'll do that.

A few more questions: it would be nice if I could could change the text on the block button from "Go" to something like "Search for Parts". Short of changing code do you have any plans to make the text configurable?

It looks like you're using a view to display the "Go" search results. How would you recommend changing the display of the results? Instead of full text I'd like to create a table of titles and product images.

Another minor tweak suggestion:

As suggested above I tagged some content with a start model year, but no end model year. I modified my part content type to display the year/make/model field info on the tagged part. I hoped to see something like "1967 Ford Mustang", but each row in the list includes an end year ("1967-1967 Ford Mustang") and links to the make and model (see attached image). If no end year is listed I'd prefer to not see an end year. I'd also prefer to not have links on the make/model values, or perhaps the use of links could be optional.

Hi Ignigena,

I hope you have great idea.
The vehicle module is working fine but i cant enable the vehicle make module it is showing some installation error.

In the Readme.txt i can see the instruction of module usage .

2. Enable the Vehicle, Vehicle Make and Vehicle Model modules and configure 
settings.  Note that the Vehicle module currently serves to display the 
administrative interface for the Make and Model modules as well as placeholder 
for future features.  You must enable one or both of these modules in order to 
use the module properly.

Please fix the vehicle make issue so that i can check the vehicle functionality.

Thank you,
Srutheesh

I just ran into some "undefined variable" errors when I enabled the "Hierarchical display of year make and models" block:

Notice: Undefined variable: model_entities in /mypath/sites/all/modules/vehicle/vehicle_ymmblock/block--vehicleymm.tpl.php on line 40

Warning: Invalid argument supplied for foreach() in /mypath/sites/all/modules/vehicle/vehicle_ymmblock/block--vehicleymm.tpl.php on line 40

Notice: Undefined variable: model_entities in /mypath/sites/all/modules/vehicle/vehicle_ymmblock/block--vehicleymm.tpl.php on line 40

Warning: Invalid argument supplied for foreach() in /mypath/sites/all/modules/vehicle/vehicle_ymmblock/block--vehicleymm.tpl.php on line 40

It looks like this can happen if isset($model_query['vehicle_model']) on line 35 returns false. I have makes and models defined so I'm not sure why that would happen, but perhaps an else clause is needed. Krumo displays the value of $model_entities as "... (Array, 0 elements)". I've attached an image showing the query parameters. I believe the problem is that I don't yet have models associated with all of the defined makes.

Just so I can keep track of this better, can you submit the feature request to the Issue queue on the project? Don't want to sidetrack the project application with these very useful feature requests that I'd love to work on implementing.

Done. Thanks for the other changes.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

• vehicle_ymm_block.tpl.php: this is a template file and should not contain any logic or DB queries. Assemble the content in vehicle_ymmblock_block_view() or a helper function.

Otherwise looks RTBC to me!

no objections for more than a week, so ...

Thanks for your contribution, Ignigena!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.

Congratulations, Ignigena. I've needed a module like this one for several years and haven't had the time to even think about writing it myself. I'm glad you took on the commitment. I'm looking forward to using it "live" and will help in any way I can to make it better.

I the same warning "An illegal choice has been detected. Please contact the site administrator." My solution was to validate the form

under vehicle_ymm.module look for

$element['year'] = array(
'#type' => 'select',
'#options' => _vehicle_ymm_build_year_range_menu(0, $current_make_id, $current_model),
'#attributes' => array('class' => array('vehicle-ymm-year')),
'#theme_wrappers' => array(),
'#default_value' => $current_year,

Change to

$element['year'] = array(
'#type' => 'select',
'#options' => _vehicle_ymm_build_year_range_menu(0, $current_make_id, $current_model),
'#attributes' => array('class' => array('vehicle-ymm-year')),
'#theme_wrappers' => array(),
'#validated' => TRUE,
'#default_value' => $current_year,

Let me know if it works out.

Sure, but I'm not sure where to post it. Newbie here ;D

*Update: I figured it out. Thanks.

Added project review link.