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
Comment | File | Size | Author |
---|---|---|---|
#28 | better_watchdog_ui-class-and-menu-fixes-b.patch | 5.56 KB | tinker |
watchdog.png | 66.43 KB | c_lehel |
Comments
Comment #1
arun_ispg CreditAttribution: arun_ispg commentedHi,
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
Comment #2
tinker CreditAttribution: tinker commented@c_lehel,
Comment #3
tinker CreditAttribution: tinker commentedUninstalling module leaves default watchdog log page unuseable. Module should revert all setting when disabled or unistalled.
Comment #4
c_lehel CreditAttribution: c_lehel commented@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!
Comment #5
c_lehel CreditAttribution: c_lehel commented@tinker
Thank You for the review!
see entity_example from Examples
Comment #6
c_lehel CreditAttribution: c_lehel commentedComment #7
PA robot CreditAttribution: PA robot commentedWe 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.
Comment #8
reszliHi 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
Comment #9
tinker CreditAttribution: tinker commented@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
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:
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.
Comment #10
reszliI 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';
Comment #11
tinker CreditAttribution: tinker commented@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.
Comment #12
reszliNaming
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
Comment #13
tinker CreditAttribution: tinker commentedSorry 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.
Comment #14
stBorchertSetting 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
watchdog_title()
: (prefix with module name, )use<?php return t('Details #!wid', array('!wid' => $watchdog->wid)); ?>
Comment #15
tinker CreditAttribution: tinker commentedI unistalled the module and when installing again using latest code:
When viewing admin/reports/dblog i get more errors. Some of these are repeated many times.
No log entries are listed.
Comment #16
tinker CreditAttribution: tinker commented@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().
Comment #17
tinker CreditAttribution: tinker commented@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.
Comment #18
c_lehel CreditAttribution: c_lehel commented@stBorchert
Thanks for the review!
I've modified what You specified in the notes.
Regarding naming:
see entity_example from Examples, where module name is
entity_example
but because the declared entity is namedentity_example_basic
, function naming follows entity namingComment #19
c_lehel CreditAttribution: c_lehel commentedComment #20
tinker CreditAttribution: tinker commented@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.
Comment #21
D34dMan CreditAttribution: D34dMan commentedPlease 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.
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.
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.
Comment #22
reszli+ re. watchdog entity
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:
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
Comment #23
tinker CreditAttribution: tinker commented@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.
Comment #24
PA robot CreditAttribution: PA robot commentedClosing 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.
Comment #25
c_lehel CreditAttribution: c_lehel commentedComment #26
c_lehel CreditAttribution: c_lehel commentedI'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!
Comment #27
kscheirerIn 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?
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.
Comment #28
tinker CreditAttribution: tinker commentedGreat work on this module. Really wanted this to be RTBC but I found a few issues:
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.
Comment #29
c_lehel CreditAttribution: c_lehel commented1-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].
Comment #30
kscheirer+1 to tinker for providing a patch along with feedback!
Comment #30.0
kscheireradded 2 review comment links
Comment #31
c_lehel CreditAttribution: c_lehel commentedAdded review bonus tag
Comment #31.0
c_lehel CreditAttribution: c_lehel commentedAdded review link http://drupal.org/node/2036957#comment-7628867
Comment #32
klausiRemoving 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.
Comment #32.0
klausiremoved automated review
Comment #33
c_lehel CreditAttribution: c_lehel commentedI've done another 3 manual reviews, added PAReview: review bonus tag
Comment #34
kscheirerWell, 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.
Comment #35
D34dMan CreditAttribution: D34dMan commentedcongrats c_lehel :)
Comment #36.0
(not verified) CreditAttribution: commentedadded 3more links to manual reviews