Hi there

I've created the following project. We've been using and developing it for some months.

https://www.drupal.org/sandbox/clancyhood/2570687

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/clancyhood/2570687.git ynot

It basically provides a means to allow developers to define blocks using simple text files. It does this by providing some default locations (conf_path()/content and theme path/ynot) for them to place .inc files in, which are discovered on clearing caches. These files may have a YAML header for config options and may or may not contain PHP depending on the setting of a YAML config option.

Since only those with direct access to the whole site's file system (and so PHP in any case) may create these blocks there are no particular drupal permissions required to do so - YNot only actually provides the ability to call hook_block_info (and similar) via a YAML header, removing the necessity to create a custom module to define a custom block, and so drupal's existing security is at play for each of these.

The resulting security considerations are fairly minor, in that by default YNot files are made available only as HTML blocks (not PHP). For non-administrators of such things as blocks, content etc to gain any kind of access to a YNot file they must first have access to token-parsing text format and a YNot file that has been specifically, deliberately exposed to the token system. I've considered putting some kind of health warning on the module page but it's quite an edge case and seems fairly straightforward common sense not to deliberately expose tokens that you don't wish to expose.

One specific security concern was to ensure that PHP was always processed prior to other text processes to prevent any kind of injection. This has been done, and can be reviewed in YnotFile::output() at about line 453 of ynot.module

Unfortunately I didn't forsee this review process (doh!) and have left it a bit late - I've a colleague going to Barcelona to evangelise about its usefulness at Drupalcon.

The code passes the coder project automated review. It could have fuller inline comments but the README documentation's pretty thorough

Thanks

Clancy Hood

CommentFileSizeAuthor
#11 ynotcs.txt6.75 KBdeveloperchris

Comments

clancyhood created an issue. See original summary.

PA robot’s picture

PA robot commented
Status: Active » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxclancyhood2570687git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

formatC'vt’s picture

formatC'vt commented
Title: YNot module » [D7] YNot module
Category: Support request » Task
Issue summary: View changes
clancyhood’s picture

clancyhood commented

Pareview seems pretty happy now http://pareview.sh/pareview/httpgitdrupalorgsandboxclancyhood2570687git

  • The .inc files contain YAML headers and cannot have file blocks. They are more like template files so the warnings and errors are erroneous.
  • Undefined variable warning are again due to the inc files being more like template files (they aren't, however, and .inc seems most suitable)
  • The "unused variables" are in loops and lists, i.e. their existence uses no memory and greatly improves readability.
clancyhood’s picture

clancyhood commented
Status: Needs work » Needs review
clancyhood’s picture

clancyhood commented

I've changed the status to "needs review". Is this right?

Digging around in various sources via Google, I've failed to spot advice as to how to further this.

developerchris’s picture

developerchris commented

Please take a look at..
https://www.drupal.org/node/1975228

This explains how to improve your chances of being reviewed in a 'more' timely manner. reviews can take an inordinate amount of time otherwise.

developerchris’s picture

developerchris commented
Status: Needs review » Needs work
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review

@DeveloperChris: you have not identified any blocking problems with this application? Anything that you found or should this be RTBC instead?

developerchris’s picture

developerchris commented

@Klausi

No I haven't reviewed the code. I was only advising how to improve the chance of a review happening. The change of status was to indicate to the developer the 'need' for more action on his behalf. If this was wrong I apologise.

developerchris’s picture

developerchris commented
StatusFileSize
new ynotcs.txt6.75 KB

pareviewcs has returned a few issues which should be attended to (see attached)

Pareview seems pretty happy now http://pareview.sh/pareview/httpgitdrupalorgsandboxclancyhood2570687git

The .inc files contain YAML headers and cannot have file blocks. They are more like template files so the warnings and errors are erroneous.
Undefined variable warning are again due to the inc files being more like template files (they aren't, however, and .inc seems most suitable)
The "unused variables" are in loops and lists, i.e. their existence uses no memory and greatly improves readability.

The readme file needs to conform to best practice (even if you don't agree with it like me, I mean 80 characters really [hellooo it's 2016 people] , the tool doesn't even conform to its own 80 character limit! ;)
https://www.drupal.org/node/2181737

Perhaps .inc may not be the best choice of ext for the included files? maybe .ynot or .yml .yaml or even .yam .yami .yamli this is because they use a specific syntax

The unused variables is a coding issue and should be resolved. If the purpose is to make it more readable a comment is far more appropriate e.g.

// test if we have a ynot file
if (YnotController::delta($delta)) {
...
}

rather than an obscure and prone to errors

if ($ynotfile = YnotController::delta($delta)) {
...
}

This is far clearer and prevents modern IDE's from polluting the workspace with warnings

Other stuff
Your introduction should provide details of what requirements are needed to install this module. For example the code relies on PHP-YAML it may also need a specific version of that library

developerchris’s picture

developerchris commented
Status: Needs review » Needs work
PA robot’s picture

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

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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