Block background module adds background images to block. User can upload background images to each block in block configuration page. We can change the background images of each block and showcase it in site.

Background images can be changed for
1.Newly created blocks.
2.View blocks
3.Core blocks (search,recent comments).

Drupal Sandbox: https://www.drupal.org/sandbox/sarahravikumar/2511202

Drupal: 7.3x

Git Clone Url : git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sarahravikumar/2511202.git block_background

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Saraswathi Ravikumar’s picture

Issue summary: View changes
Issue tags: +drupal7
Saraswathi Ravikumar’s picture

Title: Block Background » [D7]Block Background
PA robot’s picture

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.

wolffereast’s picture

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxsarahravikumar2511202git

When I clone your module using git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sarahravikumar/2511202.git block_background I only received an info file. Is the rest of the module yet to be committed?

klausi’s picture

Assigned: Saraswathi Ravikumar » Unassigned
Status: Needs review » Needs work

yep, please commit and push your code to git.

Saraswathi Ravikumar’s picture

Issue summary: View changes

Sorry for the inconvience caused.
I have added the git url for the project.

Saraswathi Ravikumar’s picture

Status: Needs work » Needs review
das.gautam’s picture

Status: Needs review » Needs work

Hi,

Below points can improve thereview process.
Please follow instructions:
1)Provide correct Git clone Url,Y ou can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".
2)If there are any related modules please mention how this module is different.(Recommended)
3)Create a better project page using this reference https://www.drupal.org/node/997024.
Reference : https://www.drupal.org/node/1011698.

Saraswathi Ravikumar’s picture

Issue summary: View changes
Saraswathi Ravikumar’s picture

Hi,

Thank You for pointing me in right direction.

This is a simple module to change the background image of each block in block create page. user can change the image whenever needed in backend.

Saraswathi Ravikumar’s picture

Status: Needs work » Needs review
das.gautam’s picture

This code should be in install file.

//Create a new directory to store the images
    $mydir = 'public://block_background'; 
    file_prepare_directory($mydir, FILE_CREATE_DIRECTORY);
    $mydir = is_writable($mydir);

Also in block_background_form_submit(), there is no need to check form_id.
if ($form_state['values']['form_id'] == 'block_admin_configure' || $form_state['values']['form_id'] == 'block_add_block_form')

Pravin Ajaaz’s picture

Manual Review:

1.Please remove the HTML tag (strong) in description property of .info file

2. In your .info file, remove Information added by drupal.org packaging script on 2015-03-23 and also the lines behind it. It will be automatically added by drupal.org when the project is promoted.

3. Please add an README.txt document for your module. Reference: https://www.drupal.org/node/2181737

4. In your .module file please remove the closing ?> PHP tag and also FILE_REPLACE_EXISTS variable is not defined and it throws some warnings when the uploaded image is altered.

5. We can able to upload any file to the block_background field. Please to restrict to image type files by adding #upload_validators attribute. For example:

    $form['settings']['block_background'] = array(
      '#type' => 'managed_file',
      '#title' => t('Background image'),
      '#description' => t('Adds the background images to block.'),
      '#upload_location'    => "public://block_background",
      '#default_value' => isset($block->block_background) ? $block->block_background : '',
      '#upload_validators' => array(
        'file_validate_extensions' => array('gif png jpg jpeg'),
        // Pass the maximum file size in bytes
        'file_validate_size' => array(MAX_FILE_SIZE*1024*1024),
      ),
      '#size' => 40,
    );
rukayya’s picture

FileSize
154.31 KB

I am getting the Notice Notice: Use of undefined constant FILE_REPLACE_EXISTS - assumed 'FILE_REPLACE_EXISTS' in block_background_form_submit() after enabling and creating a block.
Find the attachment for you info

rukayya’s picture

FileSize
164.17 KB

I can upload the PDFs as a background which is not acceptable.Find the attachment for the proof.

rukayya’s picture

please configure Drupal Coding standard and remove the HTML tags from .info files.
Also your .module file should not be ended with ?>

Saraswathi Ravikumar’s picture

Hi All,

Thanks for your support and time for reviewing my module.

Following changes have made:
1. Added upload validators. Only images can be uploaded.
2. Removed unwanted html and php tag at end in module file.
3. Moved file create code in .install file

wolffereast’s picture

Automated Review

pareview.sh shows several minor code standards violations http://pareview.sh/pareview/httpgitdrupalorgsandboxsarahravikumar2511202git

coder found 3 issues though they overlap with items pointed out by pareview

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation. This module is similar to Background Images Formatter but it fills a slightly different niche in that it is block specific. You might be able to recreate this modules functionality using the images formatter module but it wouldnt happen out of the box.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. (+)Looks like there are leftover files after removing a block. Replacing a file in the block removes the old image just fine, but deleting the block leaves the old image in place. disabling the module and uninstalling it also leaves the images in place, so an uninstall function might be called for. Looks like there is an example in the comments of the hook_block_save() documentation that shows managed file management.
  2. (+)Please add a Readme file
  3. Implementing hook_help() would improve useability.
  4. Moving the module from the 'Custom' module section to the 'Fields' or 'Media' projects might make more sense, though this might just be personal preference. You can change this by adding a `project = "[project name]" in your info file.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

ntucakovic’s picture

Status: Needs review » Needs work

CODER REVIEW

1) block_background.module#40: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

2) block_background.module#87: missing space after comma

3) block_background.install#93: the final ?> should be omitted from all code files

CODING STANDARDS

Functions are well documented, but hook_help and readme.txt are missing.
Also, project page could be improved.

SUGGESTIONS

block_background.info:

1) You don't need this line: files[] = block_background.module; Every module has to have .module file.

2) Also, you don't need package information. Read more on how to write .info files here:
https://www.drupal.org/node/542202.

Name, core and description are all you need to write here.

3) This module does not make sense if block module is disabled. You should list it as a dependency:
dependencies[] = block

block_background.install:

1) I get this error on uninstall:
Warning: is_dir(): Unable to find the wrapper "publicpublic" - did you forget to enable it when you configured PHP? in file_unmanaged_delete_recursive().

http://oi61.tinypic.com/2hyc3ld.jpg

2) Why do you need hook update?

BUGS:

At first, I could not get this module to work. This was because of these parts of code:

$bid = $block->delta;
and
drupal_add_css('#block-block'.$bid.'

They generated CSS identifier as '#block-block-form', instead of '#block-search-form' that should be generated.
So, when I replaced that parts with the code below, it worked well:

$bid = $block->module . '-' . $block->delta;
and
drupal_add_css('#block-'.$bid.'

Maybe Block module or Drupal were updated in the meantime?

Saraswathi Ravikumar’s picture

Hi,
Follwing changes have been made:

1. Readme.txt has been added.
2. Minor code standards issues have been fixed.
3. All the minor bugs have been fixed.

Thank you for reviewing my code. It has helped a lot to improve the module standard.

Saraswathi Ravikumar’s picture

Status: Needs work » Needs review
Saraswathi Ravikumar’s picture

Issue summary: View changes
ravi.khetri’s picture

Manual review :

  1. Implementing hook_help() would improve useability.
  2. Use t function in line no 22 on .install file.
Saraswathi Ravikumar’s picture

Priority: Normal » Major
rrfegade’s picture

Status: Needs review » Needs work

Use t() function in all the descriptions of .install file
Please update the project page as per the guidelines https://www.drupal.org/node/997024

Saraswathi Ravikumar’s picture

Thanks for your comments
I have added t() functions for description in .install file.

Saraswathi Ravikumar’s picture

Status: Needs work » Needs review
ajalan065’s picture

Priority: Major » Normal

Hi Sarah Ravikumar,
your application is under regular review process, so should not give the tag "major".
Rolling it back to "Normal"

ajalan065’s picture

Hi sarah ravikumar,
1. Please follow the guidelines of README template.
2. Is there any use of the variable $delete_block_image in block_background.install?
3. You should use the t() function with @ or % placeholders to construct safe and translatable strings.(line 22 of your .install file).
4. Use db_query() to access and modify the values, instead of changing it directly in .install file.

Saraswathi Ravikumar’s picture

Hi

Thank you for your comments @ajalan065.

i have made changes in readme.txt .

Also made the changes you mentioned earlier.

ajalan065’s picture

Status: Needs review » Needs work

Hi sarah ravikumar,
1. README looks too short. Add the synopsis, which you have written in your project page.
2. I tried running your module, but encountered this error. http://snag.gy/ZvinA.jpg

Saraswathi Ravikumar’s picture

Hi ajalan065,

I cannot replicate the issue which you have posted before.
Can you please uninstall the module and check once.

Thanks,
Sarah.

Saraswathi Ravikumar’s picture

Status: Needs work » Needs review
ashish.verma85’s picture

Hi, Thanks for starting this module.
Few points are:
1. http://pareview.sh/pareview/httpgitdrupalorgsandboxsarahravikumar2511202git
it shows few issues, please fix them



FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
12 | WARNING | Line exceeds 80 characters; contains 88 characters
----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/block_background.module
------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 1 LINE
------------------------------------------------------------------------
102 | ERROR | [x] Expected 1 space after IF keyword; 0 found
102 | ERROR | [x] Expected 1 space before "=="; 0 found
102 | ERROR | [x] Expected 1 space after "=="; 0 found
102 | ERROR | [x] Expected 1 space after closing parenthesis; found ""
------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/block_background.install
---------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
---------------------------------------------------------------------------
22 | ERROR | Do not use t() or st() in installation phase hooks, use $t =
| | get_t() to retrieve the appropriate localization function
| | name
22 | ERROR | The second argument to watchdog() should not be enclosed
| | with t()
---------------------------------------------------------------------------

2. it is recommended to add PHP version your .info file.
3. configuration link is missing in your .info file

Saraswathi Ravikumar’s picture

HI,
Thank you for reviewing my code.
I have added the php version.
There is no specific configuration page for this module. Background image is added for each block in its configuration page.

Thanks

ajay_reddy’s picture

Hello
Your module is working fine in Drupal7.

Manual Review

Individual user account
[Yes: Follows] the guidelines for individual user accounts
No duplication
[Yes: Does not cause]
Master Branch
[Yes: Follows]
Licensing
[Yes: Follows]
3rd party assets/code
[Yes: Follows]
README.txt/README.md
[Yes: Follows]
Code long/complex enough for review
[Yes: Follows]
Secure code
[Yes: Meets the security requirements]
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
block_background.install

severity: normalreview: i18n_2Line 22: Use st() instead of t() in hook_install() [i18n_2]
watchdog('system', t('Module install: Attempt to recreate field: "%field", when it already exists.', array('%field' => $field)), WATCHDOG_WARNING);
severity: normalreview: i18n_9Line 22: The $message argument to watchdog() should NOT be enclosed within t(), so that it can be properly translated at display time. [i18n_9]
watchdog('system', t('Module install: Attempt to recreate field: "%field", when it already exists.', array('%field' => $field)), WATCHDOG_WARNING);

Issues found in pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxsarahravikumar2511202git

PA robot’s picture

Status: Needs review » Closed (duplicate)
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2540422

Project 2: https://www.drupal.org/node/2511228

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Saraswathi Ravikumar’s picture

Status: Closed (duplicate) » Needs review
PA robot’s picture

Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2511228

Project 2: https://www.drupal.org/node/2540422

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Saraswathi Ravikumar’s picture

Priority: Normal » Major
Pravin Ajaaz’s picture

Status: Needs review » Reviewed & tested by the community

I could see all the recommended changes were made, so looks RTBC to me.

Also fix the few issues reported by pareview before promoting it.

1. In .install file, line 22 (instead of using t(), use get_t() as below):

      $t =get_t();
      watchdog('system', $t('Module install: Attempt to recreate field: "%field", when it already exists.', array('%field' => $field)), WATCHDOG_WARNING);

2. Just leave one new line at the end of the file in .info file. Now there are two new lines.

3. Wrap line 18 in READMe file to make it less than 80 characters.

However these are not blockers, hence changing the status.

Saraswathi Ravikumar’s picture

Hi Pravin,

Thank you for reviewing my code.

I will make the changes you discussed before.

Thanks,
Saraswathi.

Saraswathi Ravikumar’s picture

Hi,

Please specify the steps to publish this module in drupal.org.

Thank you all for reviewing my code.

Thanks,
Saraswathi.

ajay_reddy’s picture

Hi sarah ravikumar,

After gaining permission to create full project then you can publish this module in drupal.org as full project untill then it will be in sandbox.

PA robot’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2687695

Project 2: https://www.drupal.org/node/2511228

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

apaderno’s picture

Priority: Major » Normal
Issue tags: -drupal7
Related issues: +#2687695: [D8]Currency Convert