Project URL

http://drupal.org/sandbox/c_lehel/1922356

Git instructions

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/c_lehel/1922356.git better_watchdog_ui

For Drupal

7.x and above

Description

The module creates an entity wrapper around database log entries and enhances the watchdog listing by replacing it with a view. Beside type and severity, additional filter options were introduced such as date and author.
The possibility to change the number of listed entries per page has also been added.
Database clearing has also been customized, now admins can clear only the filtered entries, if needed.
The provided view is accessible to the admins and can be fully customized as any view, to fit one's needs.

PAReview

http://ventral.org/pareview/httpgitdrupalorgsandboxclehel1922356git
I know about the class name and the function name errors, but needed to keep them as in Views module in order to override/extend.

Manual reviews of other projects

http://drupal.org/node/1961216#comment-7295432
http://drupal.org/node/2036957#comment-7628867

https://drupal.org/node/1966828#comment-7644723
https://drupal.org/node/2004124#comment-7659039
https://drupal.org/node/2050931#comment-7689259

Comments

Status:Needs review» Needs work

Hi,

I installed your module. It adds an additional time stamp filter in report page. I think you can make it more sense. Its better you replace the time stamp with actual time, and you can also provide jquery calender with those fields. Because in Table report it showing actual time not timestamp.

There are some coding issues found in automatic review.
Refer : http://ventral.org/pareview/httpgitdrupalorgsandboxclehel1922356git

@c_lehel,

  1. All functions in better_watchdog_ui.module and better_watchdog_ui.pages.inc should begin with your module name. e.g. 'better_watchdog_ui_uri' instead of 'watchdog_uri'
  2. Class names in better_watchdog_ui.module should begin with 'betterWatchdogUi' e.g. betterWatchdogUiEntity & betterWatchdogUiEntityController
  3. Is watchdog_delete($watchdog) used anywhere?
  4. $content and $items should not be in watchdog.tpl.php You should assign those as variables in a theme preprocess function.
  5. watchdog.tpl.php should be called better_watchdog_ui.tpl.php
  6. views handlers should be named better_watchdog_ui_handler_(etc) e.g. 'better_watchdog_ui_handler_field_message' not 'views_handler_field_watchdog_message' same goes for the file name.
  7. There is no need to set variables: bwui_detail_path, bwui_view_path, bwui_parameter_position as they are static and never change. Just define them as constants and remove all the variable_get, variable_set, and variable_del function calls. This will remove the need for hook_install and hook_unistall.
  8. Have a look at http://drupal.org/node/110238 on how to set module weight. You will probably want to check watchdog's current weight and set your's higher. Your sql is wrong and should go in hook_install.

Uninstalling module leaves default watchdog log page unuseable. Module should revert all setting when disabled or unistalled.

@arun_ispg

Hello.
The date filter actually filters by date (You can see a suggested format), not (only) timestamp - I've corrected the label. You also can add your own filters if needed using Views UI.
I have run the automatic review myself, You can see my comment in the application, and You can also read the explanation.
Thank You for the review!

@tinker

Thank You for the review!

  1. The created entity was given 'watchdog' as a name, that's why naming changes;
    see entity_example from Examples
  2. Entity classes - see 1.
  3. watchdog_delete - possible later use not excluded, so I've left it in the code
  4. Introduced hook_preprocess_HOOK() -> better_watchdog_ui_preprocess_watchdog
  5. Template - see 1.
  6. Renamed view handlers
  7. Removed variable set/get, left only constant
  8. Set module weight - rewrote, also added hook_disable, to reset weight after module disable. This also solves the watchdog page reset, which You mentioned in Your second comment

Status:Needs work» Needs review

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Status:Needs review» Reviewed & tested by the community

Hi c_lehel,
this is a really nice module, thanks!
I will definitely use it once released!

+ Coder module found no problems with the code
+ PAReview has some errors regarding class/method naming conventions,
but I see your points 1. and 2. and Views module uses the same naming structure
+ regarding 3. entity functions are needed for the proper function of other modules, in this case VBO
+ I see the rest of the points above were addressed and solved
(I'm wondering if moving all entity related function to a sub-module named accordingly would make more sense and would solve the PAReview result clean)

some UI improvments I could suggest for the future
+ I would put the WBO column first as usual (severity could go last for ex.)
+ some CSS would make the filters look a bit more consistent and organized
+ replacing the 'severity' labels with small images as on the original dblog display would spare some space on the list
+ if you add the severity as a class to the table rows. the background color of each row could be changed to red/yellow/etc accordingly
+ making sure the date fits into one row for easier reading
+ add jQuery datetime picker for date filter fields

Status:Reviewed & tested by the community» Needs work

@c_lehel, Good going on the changes you made. I wrote a lot of stuff here and then realized a flaw in your design. Perhaps you should skip to Flaw in design and Recommendations sections.

Sorry perhaps I was unclear about naming convention. Naming follows module name and not entity name. In the Examples module you will notice that the module is called "entity_example" and all functions defined within it are called entity_example_something(). You have chosen "better_watchdog_ui" as your module name so to avoid function name conflicts you should follow suit. Any function called watchdog_something() needs to be renamed.

Enity name should follow module name. In entity_example module the entity is named "entity_example_basic"

You should not use "watchdog" as a module name since watchdog functions exist in drupal core.
See http://drupal.org/coding-standards#naming

  1. All functions in better_watchdog_ui.module and better_watchdog_ui.pages.inc should begin with your module name. e.g. 'better_watchdog_ui_uri' instead of 'watchdog_uri'
  2. Class names in better_watchdog_ui.module should begin with 'betterWatchdogUi' e.g. betterWatchdogUiEntity & betterWatchdogUiEntityController
  3. Is watchdog_delete($watchdog) used anywhere? - Drupal loads every function into memory so you should not include unused functions.
  4. watchdog.tpl.php should be called better_watchdog_ui.tpl.php - still needs rename
  5. module_load_include('inc', 'better_watchdog_ui', 'better_watchdog_ui.pages') is not required in the classes. The files are already loaded by hook_menu items.
  6. Module weight is now correctly set. You do not need to set it back on disable.

Flaw in design

I discovered the problem with the uninstall. You are overwriting system paths with BWUI_DETAIL_PATH. Modules extend functionality and should not overwrite existing functionality. If the module or drupal has the proper hooks you can 'alter' things. If the right hooks do not exist they you should provide you own versions of existing functions and pages that coexist with the existing functionality. If the end user prefers your module they can disable the stock system menu items and keep yours. You could set BWUI_DETAIL_PATH to "admin/reports/better_watchdog_ui". Then create a normal menu entry for the base path that links to your overview page that is based on dblog_overview(). Have a look at modules/dblog/dblog.admin.inc

Recommendations

Looking at the dblog.module I noiced that it does not use entities which makes me question why you do. I think you have gone a complex way to overwrite existing functionality when there is a much easier approach.

If you want to:

  • provide better filtering of log items then you can hook_form_alter() the "dblog_filter_form" form and change the form items. see hook_form_alter
  • control more about the display of the log page then you can create your own versions of dblog_overview() and dblog_event().
  • provide bulk operations you could use a tableselect and add a custom $form['submit'][] and $form['validate'][] to "dblog_filter_form" that link to your functions that check user input and perform the actions. see
  • change how the message details are displayed you can provide your version of theme_dblog_message()

Using this approach you would not need to use views, entities, classes, view bulk operations. Your only dependency would be dblog. Look at how dblog does the pages in modules/dblog/dblog.admin.inc and basically alter or create new versions of what is in there.

Status:Needs work» Reviewed & tested by the community

I do not agree with you tinker, on almost anything you just said.
(sorry, don't take it personally!)

Naming conventions

I would rather let someone more experienced reviewer/admin to decide on this,
but I'm sure is better to have sub-module "watchdog_entity" take care of all the entity stuff
then naming the entity "betterWatchdogUiEntity" just because it's used by that module
I'm sure there are naming standards for entities as well, not just for modules...

Recommendations

tinker
Drupal is all about modules, that are built on top of each other, without reinventing existing functionality.
When you say not to build on top of modules such as Views and VBO, you are breaking the principles.
Of course you can build all this without entities and views, but then you are creating another "static"/hard-coded display of your own - and not something useful and further expandable for the community.
The beauty of building it on top of Views, something so widely used and so configurable is that developers/site-builders can customize it to any extent of their needs, and they don't have to write a single line of code!

Grate example for this is the http://drupal.org/project/admin_views module with almost 10000 installs.
"Replaces administrative overview/listing pages with actual views for superior usability."
I use this on all my projects, and it gives me 100% freedom to configure the node/user listing as my project needs it, just by using Views UI.

In short: general/modular/configurable solutions are always better!

Flaw in design

the admin module simply sets the menu path for the views display, and views takes care of the menu alter/override
admin_views/admin_views_default/user.admin-people.inc line 313:
$handler->display->display_options['path'] = 'admin/people';
probably this is the best approach, and this is what c_lehel uses too
$handler->display->display_options['path'] = 'admin/reports/dblog';

@reszli, Everyone is free to have their own optinion and I don't take it personally that yours is in opposition to mine.

I do not agree with you tinker, on almost anything you just said.
Perhaps you could elaborate point by point on what I said wrong so that I know for the future?

Naming
'Watchdog' entity is already defined and in use by views_watchdog http://drupal.org/project/views_watchdog.
Try using that module and this one and see what happens. Is that a clear enough case to for proper naming convention?

Drupal is all about modules, that are built on top of each other, without reinventing existing functionality.
When you say not to build on top of modules such as Views and VBO, you are breaking the principles.

Yes I completely agree with you but I did not say 'not to build on top of modules'. I said this module uses a complex method to create one view. If the intention is to build one view then there is an easier way to do it.

admin_views does not overwrite system paths. It redirects using views. If you install the module and then disable it then the original paths will function. This is not the case with this module.

Naming

There is no entity defined in the views_watchdog module (I re-checked the latest D7 source code)

(No) Conflict

Drupal 6:
views_watchdog is available for D6, and it's the best soluiton, since there are no entities in D6
better_watchdog_ui will never be backported to D6, since there is no support for entities
= no problem here
Drupal 7:
better_watchdog_ui is a more robust solution for D7
it has all the features of views_watchdog and even more:
- defines an entity for the watchdog table,
- which by default makes it recognizable by Views
- and Views Bulk Operations
- comes with a default views already built, you just have to use it
- one can extend both the entity and the views, can build custom displays, feeds, etc.
= is a far better choice over views_watchdog for D7
this way you should never use the two modules together
(@c_lehel: this has to be stated on the project page though)

Paths

+ better_watchdog_ui uses the exact same way to override the menu paths as admin_views (see above code examples at #10)
and it works fine after enabling/disabling the module any number of times on a clean D7 install

Why complex solution

it's always better to do things the general way - you will benefit from it later
(we can continue brainstorming on this matter in private if you wish)
I already have a good idea how to build another module on top of this one
which I couldn't, if it was just a form_alter implementation without Views and VBO integration for ex.

thanks for your time,
reszli

Sorry c_lehel and reszli. I should have read the sandbox description of this module intsead of making an assumption. Yes using entities, views, and VBO does makes sense for the end goal.

Status:Reviewed & tested by the community» Needs work

Setting back to "needs work" since the automated review tool found some issues (basically with function name prefixing): http://ventral.org/pareview/httpgitdrupalorgsandboxclehel1922356git

The issues raised there about the views handler classes couldn't be solved since Views sets this as its standard and "will not change the complete API only to fit the coding standard" (quote from one of the Views maintainers ;) ).

Notes

  • You do not need to include you module file in the .info since this file is always loaded.
  • better_watchdog_ui.pages.inc, watchdog_title(): (prefix with module name, )use <?php return t('Details #!wid', array('!wid' => $watchdog->wid)); ?>

I unistalled the module and when installing again using latest code:

class_exists() expects parameter 1 to be string, array given handlers.inc:34
htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 1545 of /var/www/drupal7/includes/bootstrap.inc)

When viewing admin/reports/dblog i get more errors. Some of these are repeated many times.

class_exists() expects parameter 1 to be string, array given in _views_create_handler() (line 34 of /var/www/drupal7/sites/all/modules/views/includes/handlers.inc).
Undefined index: operator in views_handler_filter->exposed_form() (line 756 of /var/www/drupal7/sites/all/modules/views/handlers/views_handler_filter.inc).
Undefined index: type in views_handler_filter->accept_exposed_input() (line 1260 of /var/www/drupal7/sites/all/modules/views/handlers/views_handler_filter.inc).
Undefined index: severity in views_handler_filter->accept_exposed_input() (line 1260 of /var/www/drupal7/sites/all/modules/views/handlers/views_handler_filter.inc).
Undefined index: timestamp in views_handler_filter->accept_exposed_input() (line 1260 of /var/www/drupal7/sites/all/modules/views/handlers/views_handler_filter.inc).

No log entries are listed.

@c_lehel, I don't know your plans for this module but right now WatchdogEntityController and WatchdogEntity are not required.

WatchdogEntity does not extend Entity. It just copies it. The label and and uri callbacks are already set by better_watchdog_ui_entity_info() so it's redundant. You could use stock Entity (remove 'entity class' => 'WatchdogEntity' in hook_entity_info) unless you plan on actually extending the Entity.

WatchdogEntityController does not extend EntityAPIController either. It appears to be just used to set variables '$values' and '$content' that can be passed to their respective public functions 'create' and 'buildContent'.

If you do plan on actually extending the existing classes then I believe that your class should be contained in an .inc file rather then .module file so that class loading works properly.

watchdog_title() should be moved to the better_watchdog_ui.module file since all entity callbacks should be there.

I looked at other modules that implement entities and all of them used the module name in the Entity callback functions. But all of them used the module name in the entity name. If they implemented multiple entities then they used modulename_entityname_function() as the naming convention. So you could use just module name better_watchdog_ui_function() or better_watchdog_ui_watchdog_function().

@reszli, You are right views_watchdog does not define Entity Watchdog. It defines views data 'watchdog' . I don't know if this is a conflict.

better_watchdog_ui does not uses the exact same way to override the menu paths as admin_views since better_watchdog_ui uses hook_menu() to define 3 paths whereas admin_views uses views control of menu. The hook_menu may be the part that does not get uninstalled.

Status:Needs work» Needs review

@stBorchert
Thanks for the review!

I've modified what You specified in the notes.

Regarding naming:

  1. entity related functions naming - the created entity was given 'watchdog' as a name, that's why naming changes;
    see entity_example from Examples, where module name is entity_example but because the declared entity is named entity_example_basic, function naming follows entity naming
  2. views related naming - finally someone, who gets it :)

Title:Better Watchdog UI[D7] Better Watchdog UI

@c_lehel, I think you misunderstood what stBorchert said.

Setting back to "needs work" since the automated review tool found some issues (basically with function name prefixing)
This is in reference to:
function watchdog_title
function watchdog_view
function watchdog_delete_form etc.

This is not fixed in your last update.

The issues raised there about the views handler classes couldn't be solved since Views sets this as its standard and "will not change the complete API only to fit the coding standard"
Refers to:
better_watchdog_ui_handler_field_watchdog_message
better_watchdog_ui_handler_field_watchdog_uid
better_watchdog_ui_handler_filter_watchdog_severity etc.

These cannot be fixed for the reason stated. There has not been any disagreement about this. I stated this in post #2 point 6. This was the only other time it was mentioned so I don't know who you are refering to when you say "finally someone, who gets it :)" because no one has stated otherwise.

As to your point about Entity naming you ignore the fact that in the example module the entity is named after the module. Its called 'entity_example_basic' and not 'basic'. Try to find a module where this is not the case. I have not seen one.

I am not setting this back to "needs work", even though I still think it needs work, because you obviously need further confirmation from someone else.

@stBorchert perhaps you could confirm and set appropriate status.

Status:Needs review» Needs work

Please prefix the entity name with your module name

This project cannot use watchdog as entity name. Every entity declared by the module should be prefixed by module name.

Xano :: D34dMan, *everything* should be prefixed with the module name

Just in case if you are wondering who Xano is here is his profile on D.O.

tinker thanks for taking your time and reviewing the project. I really appreciate that.

I would rather let someone more experienced reviewer/admin to decide on this,

reszli, while making the above statement did you know tinker had 5+ years of experience ( according to his profile, since i had never interacted with tinker, i believe whatever given on his profie is true) and he has published modules on D.O, one of which has reported installs of 5000+ users!

Also i happen to notice that reszli and c_lehel are colleagues. So i see a conflict of interest there. I understand this might be something you ( two ) wanted to use, so you are more than welcome to use it. But when it has to come on D.O. it has to follow the standards.

Sorry for sounding rude ( if i had ) but just consider a ourself being a victim of poor module written by somebody else! I know there are those kinds of modules out there but lets try to reduce them if not eliminate them.

For example if your module is enabled and for some reason in the future, Drupal core team decides to use watchdog_delete would mean immediate conflict with your module. ( Do you expect Core team to consider naming their functions carefully, who would tell them about the conflict in the first place ?)

c_lehel, project review can be a pain sometimes. But in case you want to attract attention of senior reviewers, you can try obtaining a Pareview Review Bonus.

I would come back to review this application once the watchdog entity is renamed.

+ re. watchdog entity

Drupal core team decides to use watchdog_delete would mean immediate conflict with your module

as far as I know, if Drupal core team decides to use anything new, that would be only in D8, while this module is for D7

@D34dMan
I will defend my rights/opinions below, but I'm not willing to continue any of the personal conversation, since it does not belong to this issue

+ re. colleagues
yes, we are colleagues with c_lehel, and this information was/is public
please refer to comment #7 of this thread:

PA robot :: Also, you should get your friends, colleagues or other community members involved to review this application

Just in case if you are wondering who PA robot is here is his profile on D.O. :)

+ re. conflict of interests
if we (the two of use) wish to use this module, we can use it as a custom module
only if we wish to share it with others, then we need to apply for a full project

+ re. profile checks

- please be careful with this - we are here to review applications, not profiles
but if we are at it, let me detail it a bit more:

- yes, tinker has a nice D6 sandbox Going Down
for which the project application was posted 1 year 7 weeks and resolved 1 week 3 days ago once closed for half a year of inactivity,
but, the 5000+ used D7 module is Search API ranges and it belongs to morningtime

- you are a Design Engineer, and you have an application, a nice theme - so you are probably more into themes then modules

- so when I said that someone more experienced should review the process,
I meant that not tinker, but not even me are experienced enough to comment on this issue
and we rather leave it on "RTBC" so that someone with more exp. can review it - and I think this is still the case

bests,
reszli

@reszli, You state that you do not want to continue conversation in public and then post responses in public that continue the conversation. You warn not to review profiles and then you review mine. Perhaps you should follow you own suggestions.

Please don't say what I am or what I do when you have no clue. You are making massive assumptions on very limited publicly available information. I have been developing custom modules using Drupal for over 4 years and using other platforms for over a decade before that. Since I mainly write proprietary code my clients have rightfully elected not to publish them. Due to confidentiality agreements I often cannot publish any information about a lot of the work I do. Development is at least 80% of my business and most modules are more complex than this one.

I also review and provide patches for almost every Drupal module I have ever used. A number of times I completely re-wrote modules and made ports of modules from D6 to D7. The majority of these 100+ submission have not been attributed to me (I am not a maintainer by choice) and I am fine with that as I do not seek public recognition.

I think we are all here trying to improve the Drupal community and there is no need to get publicly judgmental. I have not commented on your experience, knowledge, or profile and I ask that you provide me with the same courtesy.

Status:Needs work» Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Status:Closed (won't fix)» Needs work

Status:Needs work» Needs review

I've fixed the uninstall issue mentioned earlier.

I've also renamed the watchdog entity, prefixing it with the module name, to avoid any eventual clashes with core.

Please review module and feel free to post any suggestions. Thanks!

Status:Needs review» Reviewed & tested by the community

In better_watchdog_ui_disable() I don't think you need to set the module's weight back down since it's already been disabled - the weight no longer matters. Are you sure you have the right comments for the view status, they seem backwards? if TRUE means disable, shouldn't installation set it to FALSE, and disable set it to TRUE?

<?php
function better_watchdog_ui_<em>enable</em>() {
...
 
// TRUE means disable.
 
$views_status['better_watchdog_ui_view'] = TRUE;
}
function
better_watchdog_ui_<em>disable</em>() {
...
 
// TRUE means disable.
 
$views_status['better_watchdog_ui_view'] = FALSE;
}
?>

Use single-quotes in your defines.

This looks like a great module! The watchdog interface has been fairly poor for a long time now. I didn't know about views_watchdog either. Is there a way to have the two modules work together?

----
Top Shelf Modules - Enterprise modules from the community for the community.

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new5.56 KB

Great work on this module. Really wanted this to be RTBC but I found a few issues:

  1. Move better_watchdog_ui_watchdog_title() from pages.inc to main module file. Having it in the pages.inc forces that file to be loaded when the entity is loaded which adds unnecessary overhead.
  2. When 1 is done you do not need the module_load_include() in WatchdogEntity class which again just adds overhead.
  3. You also do not need to define defaultLabel() and defaultUri() in WatchdogEntity class since the 'label callback' and 'uri callback' already set these.
  4. So when you get rid of all of the above you no longer need WatchDogEntity since it is the same as stock Entity. So you can set 'entity class' => 'Entity' in hook_entity_info() and delete WatchDogEntity class.
  5. The 'label callback' normally displays the name of the loaded entity. You have this set to return 'Details #!wid'. This may be better as 'Watchdog #!wid'
  6. There are still problems with menu entries probably due to module weight. The 'title callback' is not set properly after uninstall and reinstall. The simple fix is to use a different path for your watchdog detail page and not try to overwrite the default path 'admin/reports/event/%'.
  7. If you use your own detail path then you can get rid of the module weight changes in better_watchdog_ui_enable() and better_watchdog_ui_disable()
  8. I believe you wanted the 'admin/reports/event/%/delete' path as a MENU_LOCAL_TASK but that is not set. I don't see a need for this path at all since you provide a delete button on the detail page.
  9. One last tiny thing, the watchdog link field (when added) displays link code. I believe you need a different views handler than views_handler_field_url.

I have attached a patch with all the changes apart from the last point. In the patch I set BETTER_WATCHDOG_UI_DETAIL_PATH to 'admin/reports/bwui_event' but you can set this to any unused path. I leave the 'Delete' local task menu in the patch so you can see it work but it should probably be removed. If you remove the 'Delete' MENU_LOCAL_TASK then you can also remove the 'View' MENU_DEFAULT_LOCAL_TASK.

Status:Needs work» Reviewed & tested by the community

1-4 - the classes WatchdogEntity and WatchdogEntityController were left in the code for the purpose of later coding
5 - I've changed the name from 'Details' to 'Details #no' on purpose
6-8 - my scope was to override the system dblog path
9 - will look into it

I'm setting back status to "reviewed & tested by the community" [as it was before] since the above issues 'are surely not application blockers' [quote from kalusi].

+1 to tinker for providing a patch along with feedback!

Issue summary:View changes

added 2 review comment links

Added review bonus tag

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Issue summary:View changes

removed automated review

I've done another 3 manual reviews, added PAReview: review bonus tag

Status:Reviewed & tested by the community» Fixed

Well, all the things I mentioned in #27 still apply, but all the issues mentioned there and in #28 seem minor.

Thanks for your contribution, c_lehel!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.

congrats c_lehel :)

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

Issue summary:View changes

added 3more links to manual reviews