http://drupal.org/sandbox/DizzyC/1905296
git clone http://git.drupal.org/sandbox/DizzyC/1905296.git phpx_tools
Drupal 7 compatible.
PHP Execute Extended Tools is meant to be an extension to Devel's PHP Execute PHP page (/devel/php)
PHP X Tools adds a History section to the devel execute page as well as a Saved Script section. It keeps a history off all scripts executed on the devel/php page so you can rerun or review them at any time. It also adds the ability to save and load custom scripts so you can keep a collection of scripts that you want to be able to load and rerun at any time. (Can be some testing scripts, import scripts, deploy scrips or any other snippet that you use often like setting/getting a variable, checking the theme registry or anything else.)
Optionally it integrates with the 'CodeMirror' library (http://codemirror.net/) transforming the plain textarea into a lightweight code editor supporting syntax highlighting, code formatting, code folding, matching brackets, search and replace and some more
For a quick (and limited) test run of the tool:
You can use simplytest.me.
Go to: http://simplytest.me/project/1905296/7.x-1.x
Then click on 'Launch Sandbox' and wait for your sandbox to be set up.
Log in using admin/admin.
Go to deval/php
Play around. :)
Important Notice: If you use simplytest.me you will not be able to use codemirror integration which gives the module all it's fanciness :) I recommend you install the module on a local sandbox and download the codemirror library as explained in the README.txt for a complete experience.
Comments
Comment #1
AbhijeetKalsi CreditAttribution: AbhijeetKalsi commentedHi
there very few error related to space & indentation, please check it.
http://ventral.org/pareview/httpgitdrupalorgsandboxdizzyc1905296git
Comment #2
DizzyC CreditAttribution: DizzyC commentedHi abhijeetkalsi,
Thanks for taking a look.
I knew about those but they were just warnings not errors and they were not related to the code but to a custom code folding tag that my IDE uses.
I removed those nevertheless so now the PAReview.sh report is squeeky clean :)
http://ventral.org/pareview/httpgitdrupalorgsandboxdizzyc1905296git
Thanks!
Comment #3
alexmoreno CreditAttribution: alexmoreno commentedGood morning DizzyC,
thanks for your contribution in Drupal.org.
The module has a very nice looking, it uses nicely different files to keep everything under order, js and css in two separate folders, ... just a couple of things to help you to improve it a bit if possible.
The .install file has an schema, but you don't remove the tables when the module is uninstalled, do you?
The code is well documented, as Doxygen conventions. I would just add some more comments between the functions itself. It helps sometimes to understand better what it is happening in a more low level.
Lastly, this piece of code:
I just wonder if it would be better placed in the hook_init instead of a hook_form?
Thank you again for your effort.
Comment #4
monymirzathere very few error related to commenting, please check it.
http://ventral.org/pareview/httpgitdrupalorgsandboxdizzyc1905296git
Comment #5
DizzyC CreditAttribution: DizzyC commentedHi urwen,
Thanks for taking the time to review my code.
Adding and removing of tables defined in hook_schema is done automatically in Drupal 7, no need to manually do it on hook_install or hook_uninstall as it was in Drupal 6.
Here is a quote from hook_schema documentation page (http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...)
I added some more inline comments in my functions. Thanks for the tip.
I do not want those javascript and css files to be included on every page and that is what would happen if I place them in hook_init. I only need those files to be included when that particular form is rendered, that is why I added them to the '#attached' key of the form render array. As far as I know that is what the '#attached' key was introduced for in render arrays.
Thanks again for your comments, let me know if there is something else.
Comment #6
DizzyC CreditAttribution: DizzyC commentedmonymirza, I fixed the comments issues, PAReview output is clean again.
Comment #7
vladimir-m CreditAttribution: vladimir-m commentedHello DizzyC ,
Thank you for great module.
1. Please provide non-maintainer link to git repo. For example: "Git: git clone http://git.drupal.org/sandbox/DizzyC/1905296.git php_execute_extended_tools"
2. In file phpx_tools.userscript.inc at line: 156, Include 'No userscript.' text to t() function.
3. In file phpx_tools.module at lines: 40, 56, 72, 73, 85, 193, 196, 199, 205, 208, 211 ... use t() function http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7 and take a look to http://drupal.org/writing-secure-code
Comment #8
DizzyC CreditAttribution: DizzyC commentedVladimir,
thank you very much for taking the time and actually looking at the code.
I fixed all the issues you found. Thank you for making me aware of those.
- Changed the git link in the issue description to a public accessible link.
- Hunted down untranslatable strings and wrapped them in t().
- Added escaping to user input.
Please let me know if you find any other issues.
Thank you.
Comment #9
DizzyC CreditAttribution: DizzyC commented-- duplicate, deleted --
Comment #10
DizzyC CreditAttribution: DizzyC commented-- duplicate, deleted --
Comment #11
DizzyC CreditAttribution: DizzyC commented-- duplicate, deleted --
PS: caching issue? submitted a comment multiple times 'cause it was not showing up. Showed up later.. multiple times :)
Comment #12
alexmoreno CreditAttribution: alexmoreno commenteddon't resubmit a node or comment, it was probably saved even the page was not being shown. It is a common problem in Dr6, I think Dr7 has this solved avoiding duplicates.
Comment #13
DizzyC CreditAttribution: DizzyC commentedGot it, thanks urwen.
Could you also take a look at the code maybe? :)
Review process seems to be kinda slow in this queue.
I would be grateful for any observations.
Comment #14
klausiWe are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)
Comment #15
lathanRight love this thing!
- When one clicks "save and execute" on a script I have saved all ready aka the same name, id want the thing to update the script already saved, else I have to first delete it and then save it again... loosing the work i just did. This tool is supposed to help me with not doing that right. So maybe a warning message to the user saying you will overwrite the old script with a yes no on it before over writing it. Than can most probably be a feature request.
Quick scan over the code only issue I can spot quickly is:
- in phpx_tools_form_submit there are extra line breaks on the case statement.
The code looks good and from playing around with it, this seems like a pretty cool module. A definite +1 from me!
Comment #16
rooby CreditAttribution: rooby commentedI like the idea of this module.
Here is a review just based on me reading the code. I have not yet actually used the module and I haven't taken the time to actually understand what the js is doing etc.
Sorry if any of these have alredy been mentioned:
$form['#submit'] = array_diff($form['#submit'], array('devel_execute_form_submit'));
warrants a comment.// <editor-fold desc=" --- CRUD --- ">
? Not saying they are wrong, I'm just unfamiliar with them?use:
Comment #17
DizzyC CreditAttribution: DizzyC commented@jucallme
That is on my TODO list. I will add a overwrite confirmation dialog. Just did not come around the time to do it yet.
Yeah, I don't like cluttered code and sometimes use a blank line to separate blocks of code.
But I did some refactoring and the whole switch block is gone now.
Thank you very much. I was wondering if the module would be useful to someone else but me. It was lying around on my computer for over a year until I decided to try and post on drupal.org. Was not sure if people might use it. But it seams pretty hard to get through code review right now :) It's a pretty busy queue.
Comment #18
DizzyC CreditAttribution: DizzyC commented@rooby
First of all, thanks for taking the time to review my code.
Here are my answers to your points:
1. Fixed
2. Fixed,
Renamed to simple 'id'.
3. That is just a way of formatting arrays, in columns, so keys and values are aligned in a column.
I find this is the most readable formatting option for arrays and it does not contravene with Drupal coding standards for arrays.
Drupal coding standards state there should be spaces around '=>' so you don't end up with cluttered code, but they do not state there should be only one space on either side.
That being said, if this is going to become a problem in contributing this module I am willing to change my formatting preferences.
4. Good observation. Thank you. I changed the required permission from 'access devel information' to 'execute php code'.
5. You are right. Thanks. Done.
6. Right. I just found the name to be very meaningless, bad naming :)
I removed that line, the button is left alone. If it ain't broke, don't fix it, right?
7. Well, yes, it is personal preference and I personally don't like cluttered code. I tend to break code into blocks delimited by a blank line (where it makes sense)
But again, if this is a problem I can change it.
8. Yeah. Well generally if you look through code and find a peace of code needs an explanation / a comment there probably is a problem with that code. Clean code should be self explanatory.
And that line of code was actually kind of a hack. it was removing devel's default submit handler so the code does not execute but is just saved.
I refactored the validation and execution handlers a bit and that line of code is completely gone now.
Thanks for pointing me in that direction.
9. Well I've tried fixing all the full stop in comments. PAReview does not give me any warnings regarding this issue so I hope I have them all.
10. Well the submit handler was rewritten, the whole switch statement is gone and also is the blank line. But again, this is a personal preference and I find it easier to read if code is not that cluttered and structured.
11. See 9.
12. Right again. I refactored the CRUD functions not to use mixed variables as arguments. I agree it is less confusing this way.
13. That is markup that is used to create custom folding blocks, so for code organization. They are recognised by some of the important IDEs like NetBeans and PHP Storm. (I'm using both)
14. The table was already wrapped in a wrapper div that has a custom class that can be used for theming. Nevertheless I added a custom class on the table also.
15. Well adding a confirm page was already on my TODO list so it is implemented now.
16. Done :)
17. Well sadly the !important keywords are necessary as I am overriding some behaviour defined in the external codemirror library. I found no other way to make it behave in the devel execute page. But I am not a designer of course. I'm gonna ask a designer to take a look.
18. Done.
19. Done
Thanks again!
Let me know you thoughts!
Comment #19
mitchell CreditAttribution: mitchell commentedI tried to test your module, but it's missing the devel dependency, so simplytest.me would not satisfy the dependencies.
Looking forward to trying it.
Comment #19.0
mitchell CreditAttribution: mitchell commentedchanging git repo link to a non-maintainer link publicly accessible
Comment #20
DizzyC CreditAttribution: DizzyC commented@mitchell
Great idea using simplytest.me! Thanks.
I added a link to simplytest to the issue description too.
You can now use: http://simplytest.me/project/1905296/7.x-1.x
to get a sandbox to play around with the module. (dependencies are fixed.)
BUT be aware that if you use simplytest.me you will not have codemirror integration which is nice to have :)
So if you like it on simplytest.me i suggest you give it a spin on a local sandbox and also download the codemirror library for added fanciness like code formatting, syntax highlighting, bracket matching, search and replace, code folding...
Comment #21
DizzyC CreditAttribution: DizzyC commentedComment #22
rooby CreditAttribution: rooby commented@mitchell: It has the devel dependency.
@DizzyC:
Looks good.
3. It's not clear in the standards and I agree it comes down to personal preference. I certainly wouldn't hold up a review because of it.
6. Yeah, really it should be fixed in the devel module itself.
7. Again, I don't mind that much, it's presonal preference again. Just trying to sway people to my preference :)
9. There are still a number of comment lines without full stops in @param & @return descriptions for almost all functions with @param & @return descriptions. This is super minor though and I wouldn't "Needs work" it or that.
12. There are still references to mixed params/returns at phpx_tools_history_load(), phpx_tools_get_history(), phpx_tools_userscript_load(), phpx_tools_get_userscript(). Again, super minor.
14. Sorry, I missed that.
The only new thing I have is the use of the LIBRARIES_PATH constant.
Constants should be name spaced for the module to avoid conflicts.
Also, hard coding sites/all/libraries is not ideal as it doesn't support install profiles, which can have libraries at profiles/PROFILE_NAME/libraries or multi site installs, which can have libraries at sites/SITENAME/libraries.
The best practice and easiest solution for this type of thing is to require the Libraries API module and use it for registering and loading your library, see http://drupal.org/node/1342238 - This is not a hard requirement but is a better solution.
I have now also actually tried it out and it works well. F11 full screen for code mirror support didn't work for me in Chrome but did in Firefox. In chrome the text area folded up and nothing else happened. (the code mirror functionality is awesome :))
Since using it I have a couple of other functionality related suggestions but I don't want to derail this with feature request type things like I have seen happen with other reviews. So I will open issues in the module issue queue.
Other minor notes I have are that the info file has no newline at the end and the module description in the info file and @file doc block don't say anything about the devel module. Also, the module name says nothing about the devel module, although it doesn't have to.
Comment #23
rooby CreditAttribution: rooby commentedAlso, for sanitising user input it is generally good to use drupal's check_plain() function instead of htmlspecialchars() as used in the tables.
I have opened issues in your issue queue for non-blocking tasks so as to keep this process on topic.
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 #24.0
PA robot CreditAttribution: PA robot commentedAdded simplytest.me testing instructions.
Comment #25
pratik60 CreditAttribution: pratik60 commentedHey Dizzy
This module looks genuinely great, and attaching a screenshot so that others can get a quick idea of how it looks with codemirror integrated etc
One quick suggestion, On occasions, I end up writing really long code in Devel, and there is just too much of code displayed.
On, line 147 - php_execute_extended_tools/phpx_tools.userscript.inc -: '' . substr(htmlspecialchars($s['script']),0,400) . '',
and the same could be done on /sites/all/modules/custom/php_execute_extended_tools/phpx_tools.history.inc -: line 135.
Please reopen it DIzzy...I really would like to see this as a fully fledged module.
Comment #26
rooby CreditAttribution: rooby commentedIt is really great. And there is so little to do I feel until it would be accepted.
Comment #27
kala4ekMoved here: https://www.drupal.org/project/phpx_tools
Comment #28
DizzyC CreditAttribution: DizzyC commentedHi kala4ek,
I am glad to see there is still some interest in this module.
I hope you will be able to make it more visible for other developers as I kind of drifted away from Drupal development in the past years.
Comment #29
rooby CreditAttribution: rooby commentedNice!
I always liked using this but was never able to give it enough time to help get it through this infernal process.