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

AbhijeetKalsi’s picture

Hi

there very few error related to space & indentation, please check it.
http://ventral.org/pareview/httpgitdrupalorgsandboxdizzyc1905296git

DizzyC’s picture

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

alexmoreno’s picture

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

    // If the codemirror library exists add resources.
    if (file_exists(LIBRARIES_PATH . 'codemirror')) {
[...]

I just wonder if it would be better placed in the hook_init instead of a hook_form?

Thank you again for your effort.

monymirza’s picture

Status: Needs review » Needs work

there very few error related to commenting, please check it.
http://ventral.org/pareview/httpgitdrupalorgsandboxdizzyc1905296git

DizzyC’s picture

Status: Needs work » Needs review

Hi urwen,

Thanks for taking the time to review my code.

The .install file has an schema, but you don't remove the tables when the module is uninstalled, do you?

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

The tables declared by this hook will be automatically created when the module is first enabled, and removed when the module is uninstalled. This happens before hook_install() is invoked, and after hook_uninstall() is invoked, respectively.

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.

I added some more inline comments in my functions. Thanks for the tip.

I just wonder if it would be better placed in the hook_init instead of a hook_form?

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.

DizzyC’s picture

monymirza, I fixed the comments issues, PAReview output is clean again.

vladimir-m’s picture

Status: Needs review » Needs work

Hello 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

DizzyC’s picture

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

DizzyC’s picture

Status: Needs work » Needs review

-- duplicate, deleted --

DizzyC’s picture

-- duplicate, deleted --

DizzyC’s picture

-- duplicate, deleted --
PS: caching issue? submitted a comment multiple times 'cause it was not showing up. Showed up later.. multiple times :)

alexmoreno’s picture

don'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.

DizzyC’s picture

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

klausi’s picture

We 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 :-)

lathan’s picture

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

rooby’s picture

Status: Needs review » Needs work

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

  1. Install profile @file code block doesn't have full stop at the end.
  2. Any reason why the id_phpx & php_script fields in the DB are not just called id? Think of them as php history and php script objects with an id attribute.
  3. Some of your arrays, for example in hook_menu() and your forms there are excessive spaces between the keys and =>. There should only be a single space between things.
  4. If you have to have the 'execute php code' permission to use the devel php page, does it make sense to also require the same permission to do anything with stored scripts, instead of the 'access devel information' permission, which I would argue should be separate to anything pertaining to PHP code.
  5. In phpx_tools_form_alter() there are multiple calls to drupal_get_path() in a row. Better to do something like $path = drupal_get_path() and then use $path in all the places you need it.
  6. There shouldn't be need to copy the op button to be the execute button and then unset the op button. This doesn't seem to accomplish anything (side note: I think op is a terrible name for that button but that is devel's problem.)
  7. There are often blank lines at the start of functions and after if statements. Maybe this is personal preference but I don't think these are necessary and think it reads nicer without them.
  8. I think the line in the validate function $form['#submit'] = array_diff($form['#submit'], array('devel_execute_form_submit')); warrants a comment.
  9. There are a lot of lines in the function doc blocks that have no full stops. Mostly @param descriptions.
  10. In phpx_tools_form_submit() your switch statement breaks have a blank line before, unlike elsewhere in the file. I think the extra blank line is not necessary and makes it less nice to read.
  11. Inc files alsohave no full stops at alot of comment lines, like function doc blocks and the @file comment.
  12. When documenting mixed params (like for phpx_tools_userscript_load()) they should be pipe separated types, like int|array. Also, it might be preferable to have separate params if it makes sense. For example, maybe it could be phpx_tools_userscript_load($id = NULL, $conditions = array()), where it is possible to pass conditions without an id or vice versa. For me that is better than a mixed param potentially containing two completely different pieces of data. Same goes for phpx_tools_history_load().
  13. What are these comments like // <editor-fold desc=" --- CRUD --- ">? Not saying they are wrong, I'm just unfamiliar with them?
  14. Seeing as phpx_tools_build_userscript_table() is not a theme function, maybe the table should have a class for people to style it?
  15. Security: The script deletion through the devel/php/delete-script/% menu item is a CSRF attack hole. Basically the problem is that there is no validation before deleting. The easiest work around is to utilise the drupal confirm_form (http://api.drupal.org/api/drupal/modules!system!system.module/function/c...) to confirm whether or not you want to delete the script (confirmation is recommended for any data destructive events anyway). For more information on CSRF see http://drupalscout.com/knowledge-base/introduction-cross-site-request-fo... & http://drupalscout.com/knowledge-base/protecting-your-drupal-module-agai...
  16. The empty userscripts table message could be nicer. Maybe something like "There are currently no saved user scripts." instead of "No userscript.". - Same goes for the No history message.
  17. I have not actually investigated this but in the CSS are all the !importants really necessary (the important on the body seletor looks especially scary at first glance). !important should be a last resort and is not ideal as it makes things hard for themers. Could more specific selectors be an alternative solution? It looks like you are mostly using it for codemirror stuff so I understand that if they are using important you might have to also.
  18. Again, I have not fully investigated this but should drupal behaviors be used instead of document ready in your javascript files (generally this is the case) - See http://drupal.org/node/756722#behaviors . For example, instead of:
        $('document').ready(function () {
          // My code goes here.
      

    use:

       Drupal.behaviors.MYBEHAVIOR = {
         attach: function(context, settings) {
           // My code goes here.
     
  19. In codemirror.js there are a couple of calls to jQuery() instead of $().
DizzyC’s picture

@jucallme

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

That is on my TODO list. I will add a overwrite confirmation dialog. Just did not come around the time to do it yet.

in phpx_tools_form_submit there are extra line breaks on the case statement

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.

The code looks good and from playing around with it, this seems like a pretty cool module. A definite +1 from me!

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.

DizzyC’s picture

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

mitchell’s picture

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

mitchell’s picture

Issue summary: View changes

changing git repo link to a non-maintainer link publicly accessible

DizzyC’s picture

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

DizzyC’s picture

Status: Needs work » Needs review
rooby’s picture

Status: Needs review » Needs work

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

rooby’s picture

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

PA robot’s picture

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.

PA robot’s picture

Issue summary: View changes

Added simplytest.me testing instructions.

pratik60’s picture

Hey 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

Only local images are allowed.

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.

rooby’s picture

It is really great. And there is so little to do I feel until it would be accepted.

kala4ek’s picture

DizzyC’s picture

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

rooby’s picture

Nice!
I always liked using this but was never able to give it enough time to help get it through this infernal process.