A simple way to associate an image or a slideshow to a path.

You can upload an image and set it to be shown as background image for the specified path. more images on the same path will become a slideshow (if jquery.cycle is installed) . You can also associate images to "default" , so if the path you're visiting has no images directly attached one random image (or a slideshow) from the "default" is shown. Backgrounds can also be exposed as a block. Drush commands also available.

Project
https://drupal.org/sandbox/degami/2237103
https://github.com/degami/page_background.git

GIT
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/degami/2237103.git page_background

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxdegami2237103git

CommentFileSizeAuthor
#44 page backgroung 2.png89.07 KBalokvermaei
#44 page background1.png93.26 KBalokvermaei
#44 Page background.png108.17 KBalokvermaei
#44 page background 6.png98.44 KBalokvermaei
#44 page background 4.png88.79 KBalokvermaei
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

degami’s picture

degami CreditAttribution: degami commented
Issue summary: View changes
degami’s picture

degami CreditAttribution: degami commented
Issue summary: View changes
PA robot’s picture

PA robot CreditAttribution: PA robot commented
Status: Needs review » Needs work

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

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.

degami’s picture

degami CreditAttribution: degami commented
Status: Needs work » Needs review
perennial.sky’s picture

perennial.sky CreditAttribution: perennial.sky commented

Module is not working correctly. I have just enable module and goto to config page upload photo and specific path but there is no background's image in that page but I have seen this code inside "style" tag

#page_background_container{height:300px;width:300px;overflow:hidden;position:absolute;z-index:-1;top:0;}
#page_background_container{left:50%;margin-left:-150px;}
.page_background_slide{margin:0;padding:0;height:300px;width:300px;}
.page_background_slide .img{display:block;margin:auto;}
#page_background_slide_0{background:url(http://www.innosome.com:8080/sites/default/files/page_backgrounds/536631_303193539755886_1330519231_n.jpg) no-repeat center top;width:500px;height:552px;position:absolute;top:50%;left:50%;margin-left:-250px;margin-top:-276px;}

And i m just reviewed your code not completely but some part
Please consider following points

- In this _page_background_check_access() function, Use role name instead of uid , May be site have many administrator so definitely they have different uid.

- What is the use of _page_background_check_access() function we already use permission hook.

perennial.sky’s picture

perennial.sky CreditAttribution: perennial.sky commented
Category: Task » Bug report
Status: Needs review » Needs work
degami’s picture

degami CreditAttribution: degami commented

Maybe you've assigned that image to the defaults ? in that case, as written in the module description "if the path you're visiting has no images directly attached one random image (or a slideshow) from the "default" is shown"

_page_background_check_access returns true if you have at least one of the permissions specified, (if i'm not wrong, user_access cannot take an array of permissions as parameter )
the only uid checked is uid:1 (wich normally is the admin user) , if you wish you can assing permission to the administrator role, right?

degami’s picture

degami CreditAttribution: degami commented

If i didn't understood well your problem ( i'm sorry for my english ) , another question is: does your theme render the "page_top" region? the markup is attached to that region (if you didn't expose the background as a block)

degami’s picture

degami CreditAttribution: degami commented
Category: Bug report » Task
Status: Needs work » Needs review
druppa’s picture

druppa CreditAttribution: druppa commented

Just tested this, everything seems to work as intended. Good job!

degami’s picture

degami CreditAttribution: degami commented
Status: Needs review » Reviewed & tested by the community
degami’s picture

degami CreditAttribution: degami commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work 
/**
 * Page_background_check_access().
 */
function _page_background_check_access($access) {

Missing documentation.

Though that's somewhat moot, as I don't think this function is necessary.

  if ($user->uid == 1) {
    return TRUE;
  }

user_access() already does that bypass for you.

And:

  if (!is_array($access)) {

I can't find anywhere you are calling _page_background_check_access() with an array.

Therefore, you might as well use user_access() as your menu access callback and remove this.

Finally (though also moot), $access is usually used to mean the boolean that says whether access is granted or not. Here you're using it to hold a permission string. $permission is more usual for this.

/**
 * Implements hook_update_N().
 */
function page_background_update_7000(&$sandbox) {

A newly-released module probably shouldn't have hook_update_N()s...

degami’s picture

degami CreditAttribution: degami commented
Status: Needs work » Needs review

Hi.
page_background_check_access, take a look to comment #7. Won't change it.

Hook_update: this module is new for you, not for the sites where i have it already installed.

Thanks for you review.

joachim’s picture

joachim CreditAttribution: joachim commented

You mean this:

_page_background_check_access returns true if you have at least one of the permissions specified, (if i'm not wrong, user_access cannot take an array of permissions as parameter )

Where are you using this with an array of permission? I can't see anywhere!

degami’s picture

degami CreditAttribution: degami commented

Yeah, i'm not using it in that way. You're right. Won't change it.

joachim’s picture

joachim CreditAttribution: joachim commented

What makes you think you'll need it in future? You might as well remove it, and you can always revert the commit if in the future you find you do. But if you don't, you've simplified your code and your maintenance burden.

degami’s picture

degami CreditAttribution: degami commented

Ok joachim, just for you :) used an array of permissions

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle commented
Status: Needs review » Needs work

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

README.txt exists, but does not follow the guidelines for in-project documentation and the README.txt Template.

There are also some easy to fix problems regarding coding standard reported by PAReview.sh. See http://pareview.sh/pareview/httpgitdrupalorgsandboxdegami2237103git

degami’s picture

degami CreditAttribution: degami commented
Status: Needs work » Needs review

Will change the branch after having the permission to promote my sandbox to a full project

As i read in the "Moving from a master to a major version branch" page (https://www.drupal.org/node/1127732)

...
"As long as your project is in the sandbox it can stay in the master branch as instructed or suggested by the sandbox version control page."
...

Moreover, i fixed those tremendous errors of "missing dot at the end of a comment sentence" and "the readme.md file has lines that are more than 80 characters long" that for sure were preventing the module from working as intended

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle commented

Will change the branch after having the permission to promote my sandbox to a full project.

That is your prerogative. However, you will not receive another review from me until your sandbox is set up with a version specific branch as required by the official documentation.

Btw. the text you cite as grounds for not doing it right now is not official documentation. It just a random comment made by a (IMHO) misinformed member of the community.

joachim’s picture

joachim CreditAttribution: joachim commented

> Ok joachim, just for you :) used an array of permissions

That doesn't seem to me to be a very good reason for changing how the access of your module works!

degami’s picture

degami CreditAttribution: degami commented

Hi Joachim.
Didn't change the way my module works just for you. I simply changed the hook_menu using an array of permissions ( i've added a new permission ), moreover i've verified these permissions also on the node_form and in the taxonomy_term form, as i said i didn't want to change the code

Regards

degami’s picture

degami CreditAttribution: degami commented
Status: Needs review » Reviewed & tested by the community
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle commented
Status: Reviewed & tested by the community » Needs review

Please note that you're not supposed to change the status to from "Needs review" to "Reviewed & tested by the community" (RTBC) yourself. Please see What to expect for an overview of the process.

degami’s picture

degami CreditAttribution: degami commented

Hello gisle, i know it, it was just to check if the community is still there.

Regards

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle commented

I know it, it was just to check if the community is still there.

Do you also know that every time you pull a stunt like that, you will - in effect - reposition yourself at the rear of queue?

We're quite busy reviewing here, but the queue is long and yours is not the only one that needs review. We can only do one at the time.

In the meantime, I suggest you take a look at Tips for ensuring a smooth review and the Application Checklist. Doing most the checks yourself beforehand will cut down on the number of iterations your project will need to go through to pass review, and will save you (and us!) a lot of time.

willietse’s picture

willietse CreditAttribution: willietse commented
Issue tags: +PAreview: review bonus
README.txt
No
README.txt is incomplete and needs to be substantially fleshed out.
Manual code review
  • I enable module, can not find the config page (need to click on the INDEX at the /admin page), therefore, config page moves to a certain category I suggest, for example: $items['admin/config/media/page_background'] = array ()
  • about the _page_background_check_access, why not just use the default'Access callback', only need 'access arguments'= > array ('administer page_background'),
  • A function call in place, You can use the user_access ().
Other concerns
  • I upload multiple images in the same path, if installed jQuery cycle, you can use the slide, but, I have not installed jQuery cycle, I should get some tips on the config page or'admin/reports/status'page?
  • The lack of hook_help ()
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle commented
Issue tags: -PAreview: review bonus

@willetse, please stop adding the "PAReview: review bonus" tag to a project every time you review a project.

I've no idea why you're doing it, but it is not the correct way to use this tag.

(You may change the status to "Needs work", but if you think your own review is superficial or incomplete and that it may benefit from having feedback from more reviewers, leave it at "Needs review" like you've done.)

willietse’s picture

willietse CreditAttribution: willietse commented

Sorry, I think that is the method I do not use PAReview: review bonus.

degami’s picture

degami CreditAttribution: degami commented

Moved configuration pages under /admin/config/media.
Implemented hook_help.
Added warning if Cycle library is missing in the settings page.

gbisht’s picture

gbisht CreditAttribution: gbisht commented

@degami nice module but there are some other modules (Background Images, Dynamic Background and Responsive Background Images) which provide similar type of functionality as yours. Can you please specify what new functionality your module will provide.
$cycle_path variable is unused on 1002 line - http://pareview.sh/pareview/httpgitdrupalorgsandboxdegami2237103git
Try to elaborate a little on commenting of you form, validate, submit and other function as right now you are just using function name in comment. It will also help other people to understand you code better.

degami’s picture

degami CreditAttribution: degami commented

added more elaborate function comments.
btw line 1002 was a false positive.

irfworld’s picture

irfworld CreditAttribution: irfworld commented

Hi
.inc files should be used for the menus used, instead of using all the code in a single .module file.

degami’s picture

degami CreditAttribution: degami commented

I'm pretty sick of this community. why before saying that the hook_menu should be placed in a *. inc files ( all alone? for what reason? ) we do not control where the other much popular modules implement their hook_menu ? why instead of having to worry about something that works we care so much that it is "beautiful" (for the sake of whom?)

btw, in one of my projects sites/all/modules folder

$ grep -R "_menu("

variable/variable_example/variable_example.module: * Implements hook_menu().
variable/variable_example/variable_example.module:function variable_example_menu() {
variable/variable_admin/variable_admin.module: * Implements hook_menu().
variable/variable_admin/variable_admin.module:function variable_admin_menu() {
imce/imce.module: * Implements hook_menu().
imce/imce.module:function imce_menu() {
jqueryui_theme/jqueryui_theme.module: * Implements hook_menu()
jqueryui_theme/jqueryui_theme.module:function jqueryui_theme_menu() {
jqueryui_theme/jqueryui_theme.module: * Callback function theme for hook_menu().
google_analytics/googleanalytics.module: * Implements hook_menu().
google_analytics/googleanalytics.module:function googleanalytics_menu() {
skinr/tests/skinr_test/skinr_test.module: * Implements hook_menu().
skinr/tests/skinr_test/skinr_test.module:function skinr_test_menu() {
skinr/skinr_context/skinr_context_ui.module: * Implements hook_menu().
skinr/skinr_context/skinr_context_ui.module:function skinr_context_ui_menu() {
skinr/skinr_ui.module: * Implements hook_menu().
skinr/skinr_ui.module:function skinr_ui_menu() {
skinr/CHANGELOG.txt:#908946 by sun: Fixed hook_menu() is invoked for disabled modules.
context_addassets/context_addassets.module: $core_context_items = context_ui_menu();
context_addassets/context_addassets.module: * Implements hook_menu().
context_addassets/context_addassets.module:function context_addassets_menu() {
xmlsitemap/xmlsitemap_menu/xmlsitemap_menu.module: * @see menu_edit_menu()
xmlsitemap/xmlsitemap_engines/xmlsitemap_engines.module: * Implements hook_menu().
xmlsitemap/xmlsitemap_engines/xmlsitemap_engines.module:function xmlsitemap_engines_menu() {
xmlsitemap/xmlsitemap_engines/tests/xmlsitemap_engines_test.module: * Implements hook_menu().
xmlsitemap/xmlsitemap_engines/tests/xmlsitemap_engines_test.module:function xmlsitemap_engines_test_menu() {
xmlsitemap/xmlsitemap.module: * Implements hook_menu().
xmlsitemap/xmlsitemap.module:function xmlsitemap_menu() {
xmlsitemap/xmlsitemap_custom/xmlsitemap_custom.module: * Implements hook_menu().
xmlsitemap/xmlsitemap_custom/xmlsitemap_custom.module:function xmlsitemap_custom_menu() {
token/token.module: * Implements hook_menu().
token/token.module:function token_menu() {
backup_migrate/includes/crud.inc:function backup_migrate_crud_menu() {
backup_migrate/backup_migrate.module: * Implementation of hook_menu().
backup_migrate/backup_migrate.module:function backup_migrate_menu() {
backup_migrate/backup_migrate.module: $items += backup_migrate_crud_menu();
page_background/page_background.module: * Implements hook_menu().
page_background/page_background.module:function page_background_menu() {
recaptcha/recaptcha.module: * Implements hook_menu().
recaptcha/recaptcha.module:function recaptcha_menu() {
ctools/ctools_ajax_sample/ctools_ajax_sample.module: * Implementation of hook_menu()
ctools/ctools_ajax_sample/ctools_ajax_sample.module:function ctools_ajax_sample_menu() {
ctools/ctools_ajax_sample/ctools_ajax_sample.module: $form = ctools_jump_menu(array(), $form_state, array($url => t('Jump!')), array());
ctools/ctools_plugin_example/ctools_plugin_example.module:function ctools_plugin_example_menu() {
ctools/views_content/views_content.module: * Implements hook_menu().
ctools/views_content/views_content.module:function views_content_menu() {
ctools/includes/export-ui.menu.inc: * Delegated implementation of hook_menu().
ctools/includes/export-ui.menu.inc:function ctools_export_ui_menu(&$items) {
ctools/includes/export-ui.menu.inc: $handler->hook_menu($items);
ctools/includes/context.menu.inc:function ctools_context_menu(&$items) {
ctools/includes/page-wizard.menu.inc:function ctools_page_wizard_menu(&$items) {
ctools/includes/utility.inc: * In particular, things that are only needed during hook_menu() and
ctools/includes/content.menu.inc:function ctools_content_menu(&$items) {
ctools/includes/jump-menu.inc:function ctools_jump_menu($form, &$form_state, $select, $options = array()) {
ctools/plugins/export_ui/ctools_export_ui.class.php: * hook_menu() entry point.
ctools/plugins/export_ui/ctools_export_ui.class.php: * probably call parent::hook_menu($items) and then modify as needed.
ctools/plugins/export_ui/ctools_export_ui.class.php: function hook_menu(&$items) {
ctools/plugins/content_types/page/page_primary_links.inc: $block->content = theme('links', array('links' => menu_main_menu(), 'attributes' => array('class' => 'links primary-links')));
ctools/plugins/content_types/page/page_secondary_links.inc: $block->content = theme('links', array('links' => menu_secondary_menu(), 'attributes' => array('class' => 'links secondary-links')));
ctools/bulk_export/bulk_export.module: * Implements hook_menu().
ctools/bulk_export/bulk_export.module:function bulk_export_menu() {
ctools/page_manager/page_manager.module: * Delegated implementation of hook_menu().
ctools/page_manager/page_manager.module:function page_manager_menu() {
ctools/page_manager/plugins/tasks/page.admin.inc: * Delegated implementation of hook_menu().
ctools/page_manager/plugins/tasks/page.admin.inc:function page_manager_page_menu(&$items, $task) {
ctools/page_manager/plugins/tasks/page.admin.inc:function page_manager_page_form_menu($form, &$form_state) {
ctools/ctools.module: * Note that some hooks such as _menu() or _theme() must return an array().
ctools/ctools.module: * Implements hook_menu().
ctools/ctools.module:function ctools_menu() {
ctools/ctools.module:function ctools_access_menu($access) {
ctools/CHANGELOG.txt:#999302 by troky: ctools_jump_menu() needed updating to new form parameters.
captcha/captcha_api.txt:=== Recommended: hook_menu($may_cache) ===
captcha/captcha_api.txt: * Implementation of hook_menu().
captcha/captcha_api.txt:function foo_captcha_menu($may_cache) {
captcha/image_captcha/image_captcha.module: * Implements hook_menu().
captcha/image_captcha/image_captcha.module:function image_captcha_menu() {
captcha/captcha.module: * Implementation of hook_menu().
captcha/captcha.module:function captcha_menu() {
redirect/redirect.module: * Implements hook_menu().
redirect/redirect.module:function redirect_menu() {
exchange_links/exchange_links.module: * Implements of hook_menu()
exchange_links/exchange_links.module:function exchange_links_menu() {
imagecache_actions/image_styles_admin/image_styles_admin.module: * Implements hook_menu().
imagecache_actions/image_styles_admin/image_styles_admin.module:function image_styles_admin_menu() {
imagecache_actions/tests/imagecache_testsuite.module: * Implementation of hook_menu().
imagecache_actions/tests/imagecache_testsuite.module:function imagecache_testsuite_menu() {
adminimal_admin_menu/adminimal_admin_menu.module: * Implements hook_menu().
adminimal_admin_menu/adminimal_admin_menu.module:function adminimal_admin_menu_menu() {
page_title/page_title.module: * Implement hook_menu().
page_title/page_title.module:function page_title_menu() {
google_adwords/google_adwords.module:function google_adwords_menu(){
facebookshare/facebookshare.module: * Implements hook_menu()
facebookshare/facebookshare.module:function facebookshare_menu() {
metatags_quick/metatags_quick.module: * Implements hook_menu().
metatags_quick/metatags_quick.module:function metatags_quick_menu() {
metatags_quick/metatags_quick_extra.module: * Implements hook_menu().
metatags_quick/metatags_quick_extra.module:function metatags_quick_extra_menu() {
metatags_quick/metatags_quick_import.module: * Implements hook_menu().
metatags_quick/metatags_quick_import.module:function metatags_quick_import_menu() {
i18nviews/i18nviews.module: * Implements hook_menu().
i18nviews/i18nviews.module:function i18nviews_menu() {
context/context_ui/context_ui.module: * Implementation of hook_menu().
context/context_ui/context_ui.module:function context_ui_menu() {
entity_translation/entity_translation.module: * Implements hook_menu().
entity_translation/entity_translation.module:function entity_translation_menu() {
entity_translation/entity_translation_upgrade/entity_translation_upgrade.module: * Implements hook_menu().
entity_translation/entity_translation_upgrade/entity_translation_upgrade.module:function entity_translation_upgrade_menu() {
entity_translation/entity_translation.install: // translation_menu(); entity_translation_menu_alter() needs to run after
entity_translation/CHANGELOG.txt: callback/arguments in entity_translation_menu().
google_plusone/google_plusone.module: * Implements hook_menu().
google_plusone/google_plusone.module:function google_plusone_menu() {
views/includes/view.inc: * Called to get hook_menu() information from the view and the named display handler.
views/includes/view.inc: function execute_hook_menu($display_id = NULL, &$callbacks = array()) {
views/includes/view.inc: return $this->display_handler->execute_hook_menu($callbacks);
views/views.module: * Implement hook_menu().
views/views.module:function views_menu() {
views/views.module: $result = $view->execute_hook_menu($display_id, $callbacks);
views/plugins/views_plugin_display.inc: function hook_menu() { return array(); }
views/plugins/export_ui/views_ui.class.php: function hook_menu(&$items) {
views/plugins/export_ui/views_ui.class.php: parent::hook_menu($items);
views/plugins/views_plugin_display_page.inc: function execute_hook_menu($callbacks) {
views/tests/views_access.test: $hook_menu = $view->execute_hook_menu('page_1');
views/tests/views_access.test: $hook_menu = $view->execute_hook_menu('page_1');
views/views_ui.module: * Implements hook_menu().
views/views_ui.module:function views_ui_menu() {
views/views_ui.module: * The same caution must be used when defining a hook_menu() entry with this
views/views_ui.module: * page callback as is used when defining a hook_menu() entry with the
admin_menu/admin_menu.inc:function admin_menu_links_menu($tree) {
admin_menu/admin_menu.inc: $links = array_merge($links, admin_menu_links_menu($data['below']));
admin_menu/admin_menu.inc: $children = admin_menu_links_menu($data['below']);
admin_menu/admin_menu.inc: $devel_links = admin_menu_links_menu($devel_tree);
admin_menu/admin_menu.module: * Implements hook_menu().
admin_menu/admin_menu.module:function admin_menu_menu() {
admin_menu/admin_menu.module: $content['menu'] = admin_menu_links_menu(admin_menu_tree('management'));
admin_menu/admin_menu.module: $links = admin_menu_links_menu($tree);
admin_menu/admin_menu.api.php: * @see admin_menu_links_menu()
admin_menu/CHANGELOG.txt:Added devel_admin_menu() for fast access to clear-cache, variable editor and
admin_menu/CHANGELOG.txt:Added hook_admin_menu() to allow other modules to alter admin_menu.
domain/domain_conf/domain_conf.module: * Implements hook_menu()
domain/domain_conf/domain_conf.module:function domain_conf_menu() {
domain/domain_content/domain_content.module: * Implements hook_menu()
domain/domain_content/domain_content.module:function domain_content_menu() {
domain/domain_content/domain_content.module: * The access check value passed from hook_menu().
domain/domain_nav/domain_nav.module: * Killswitch for hook_menu().
domain/domain_nav/domain_nav.module: * Implements hook_menu()
domain/domain_nav/domain_nav.module:function domain_nav_menu() {
domain/domain_nav/domain_nav.module: domain_nav_rebuild_menu();
domain/domain_nav/domain_nav.module: domain_nav_rebuild_menu();
domain/domain_nav/domain_nav.module: domain_nav_rebuild_menu();
domain/domain_nav/domain_nav.module:function domain_nav_rebuild_menu() {
domain/domain.module: * Implements hook_menu().
domain/domain.module:function domain_menu() {
domain/domain_theme/domain_theme.module: * Implements hook_menu()
domain/domain_theme/domain_theme.module:function domain_theme_menu() {
domain/domain_alias/domain_alias.module: * Implements hook_menu().
domain/domain_alias/domain_alias.module:function domain_alias_menu() {
modules_weight/modules_weight.module:* Implementation of hook_menu().
modules_weight/modules_weight.module:function modules_weight_menu(){
date/date_views/date_views.module: * Implements hook_menu().
date/date_views/date_views.module:function date_views_menu() {
date/date_popup/date_popup.module: * Implements hook_menu().
date/date_popup/date_popup.module:function date_popup_menu() {
date/date_tools/date_tools.module: * Implements hook_menu().
date/date_tools/date_tools.module:function date_tools_menu() {
date/date_repeat_field/date_repeat_field.module: * Implements hook_menu().
date/date_repeat_field/date_repeat_field.module:function date_repeat_field_menu() {
date/date_api/date_api.module: * Implements hook_menu().
date/date_api/date_api.module:function date_api_menu() {
menu_position/menu_position.module: * Implements hook_menu().
menu_position/menu_position.module:function menu_position_menu() {
lightbox2/lightbox2.module: * Implementation of hook_menu().
lightbox2/lightbox2.module:function lightbox2_menu() {
ckeditor/ckeditor.module: * Implementation of hook_menu().
ckeditor/ckeditor.module:function ckeditor_menu() {
wysiwyg/wysiwyg.module: * Implementation of hook_menu().
wysiwyg/wysiwyg.module:function wysiwyg_menu() {
wysiwyg/tests/wysiwyg_test.module: * Implements hook_menu().
wysiwyg/tests/wysiwyg_test.module:function wysiwyg_test_menu() {
wysiwyg/wysiwyg.dialog.inc: * - wysiwyg_menu() defines a custom delivery callback that replaces
wysiwyg/wysiwyg.dialog.inc: * @see wysiwyg_menu()
l10n_update/l10n_update.module: * Implements hook_menu().
l10n_update/l10n_update.module:function l10n_update_menu() {
location/contrib/location_search/location_search.module: * Implements hook_menu().
location/contrib/location_search/location_search.module:function location_search_menu() {
location/location.module: * Implements hook_menu().
location/location.module:function location_menu() {
google_fonts/google_fonts.module: * Implements hook_menu().
google_fonts/google_fonts.module:function google_fonts_menu() {
entity/entity.api.php: * - path: A path where the UI should show up as expected by hook_menu().
entity/entity.api.php: * as it is required by hook_menu().
entity/entity.api.php: * - menu wildcard: The wildcard to use in paths of the hook_menu() items.
entity/includes/entity.ui.inc: * Provides definitions for implementing hook_menu().
entity/includes/entity.ui.inc: public function hook_menu() {
entity/includes/entity.ui.inc: // Set this on the object so classes that extend hook_menu() can use it.
entity/includes/entity.ui.inc: * Provides definitions for implementing hook_menu().
entity/includes/entity.ui.inc: public function hook_menu() {
entity/includes/entity.ui.inc: $items = parent::hook_menu();
entity/includes/entity.ui.inc: * Provides definitions for implementing hook_menu().
entity/includes/entity.ui.inc: public function hook_menu() {
entity/includes/entity.ui.inc: $items = parent::hook_menu();
entity/entity.module: * Implements hook_menu().
entity/entity.module: * @see EntityDefaultUIController::hook_menu()
entity/entity.module:function entity_menu() {
entity/entity.module: $items += $controller->hook_menu();
i18n/i18n_taxonomy/i18n_taxonomy.module: * Implements hook_menu().
i18n/i18n_taxonomy/i18n_taxonomy.module:function i18n_taxonomy_menu() {
i18n/i18n_menu/i18n_menu.module: * Implements hook_menu()
i18n/i18n_menu/i18n_menu.module:function i18n_menu_menu() {
i18n/i18n_field/i18n_field.module: * Implements hook_menu().
i18n/i18n_field/i18n_field.module:function i18n_field_menu() {
i18n/i18n_field/i18n_field.module: // Create tabs for all possible bundles. From field_ui_menu().
i18n/i18n_block/i18n_block.module: * Implements hook_menu().
i18n/i18n_block/i18n_block.module:function i18n_block_menu() {
i18n/i18n.module: * Implements hook_menu().
i18n/i18n.module:function i18n_menu() {
i18n/i18n_contact/i18n_contact.module: * Implements hook_menu().
i18n/i18n_contact/i18n_contact.module:function i18n_contact_menu() {
i18n/i18n_translation/i18n_translation.module: * Implements hook_menu()
i18n/i18n_translation/i18n_translation.module:function i18n_translation_menu() {
i18n/i18n_variable/i18n_variable.module: * Implements hook_menu()
i18n/i18n_variable/i18n_variable.module:function i18n_variable_menu() {
i18n/i18n_select/i18n_select.module: * Implements hook_menu().
i18n/i18n_select/i18n_select.module:function i18n_select_menu() {
i18n/i18n_node/i18n_node.module: * Implements hook_menu().
i18n/i18n_node/i18n_node.module:function i18n_node_menu() {
i18n/i18n_string/i18n_string.module: * Implements hook_menu().
i18n/i18n_string/i18n_string.module:function i18n_string_menu() {
i18n/tests/i18n_test.module: * Implements hook_menu().
i18n/tests/i18n_test.module:function i18n_test_menu() {
i18n/i18n_path/i18n_path.module: * Implements hook_menu()
i18n/i18n_path/i18n_path.module:function i18n_path_menu() {
libraries/tests/modules/libraries_test_module/libraries_test_module.module: * Implements hook_menu().
libraries/tests/modules/libraries_test_module/libraries_test_module.module:function libraries_test_module_menu() {
selectmenu/selectmenu.module: * Implements hook_menu().
selectmenu/selectmenu.module:function selectmenu_menu() {
devel/devel.install: * Rebuild the menus since everything is defined by devel_menu().
devel/devel_node_access.module: * Implements hook_menu().
devel/devel_node_access.module:function devel_node_access_menu() {
devel/devel.module: * Implements hook_menu().
devel/devel.module:function devel_menu() {
devel/devel.module: 'description' => 'Rebuild menu based on hook_menu() and revert any custom changes. All menu items return to their default settings.',
devel/devel.module: * This is more robust than setting alter in hook_menu().
devel/devel_generate/devel_generate.module: * Implements hook_menu().
devel/devel_generate/devel_generate.module:function devel_generate_menu() {
toolbar_visibility/toolbar_visibility.module: * Implements hook_menu().
toolbar_visibility/toolbar_visibility.module:function toolbar_visibility_menu() {
pathauto/pathauto.module: * Implements hook_menu().
pathauto/pathauto.module:function pathauto_menu() {
smtp/smtp.module: * Implements hook_menu().
smtp/smtp.module:function smtp_menu() {
guestbook/guestbook.module: * Implementation of hook_menu().
guestbook/guestbook.module:function guestbook_menu() {

degami’s picture

degami CreditAttribution: degami commented

or, as I think, if you mean to say "place the page callbacks functions in a .inc file", even if the first part of my previous comment stays the same, the second becomes: "Why? Because some religion says that we should not have files with more than 100 lines, or the others get bored to read them?"

i'm so sorry for my poor english..

irfworld’s picture

irfworld CreditAttribution: irfworld commented

@Degami,

for an enabled module, all it's code from ".module" file, is loaded on each and every request so it may result in poor performance. And if we use the code in separate ".inc" file then the code is loaded only when the menu is called.

And your "hook_menu" should be declared in ".module" file only.

I hope, you don't have any doubt now.

degami’s picture

degami CreditAttribution: degami commented

Yeah, sure you're a programming guru and what else, i'm sure. But... are you talking about performances ( in the scale of microseconds perhaps ? ) on a system that just to load a single page, normally does hundreds of sql queries? that system that gives you the views module ?
have you ever seen one query created by views and any views-integrated-module out there?

i think the real poor performance is found elsewhere in drupal.
btw. maybe i'm not skilled as you in php programming, loading 2 files ( and so accessing two different inodes ) is less or more expensive than a bigger one? maybe if the bigger is really big... but here we talk about less than 100kb ( and i don't think that the systems nowadays can't stand this size )

pingwin4eg’s picture

pingwin4eg
Location Zaporizhia 🇺🇦
CreditAttribution: pingwin4eg commented

I didn't know that the project application reviews can be so funny. I'm subscribing to the issue just to see what'd be next)))

geekygnr’s picture

geekygnr CreditAttribution: geekygnr commented
Status: Needs review » Needs work

@degami, It sounds like you are frustrated with the review process and I can understand that. I am going through the same process myself and it can be hard to have others critique your hard work. Everyone here is just trying help you have the best code you possibly can and a part of that is following the established best practices.

I would highly recommend seriously considering every piece of advice offered and applying it to your project. Even if you think the change will bring little benefit the amount you will learn about the Drupal API and how to best use it will surprise you.

PAReview is showing some coding style errors. http://pareview.sh/pareview/httpgitdrupalorgsandboxdegami2237103git

It looks like the repository now has a version specific branch which is good.

I would also suggest to make things easier that when you switch the status back to Needs Review you comment on what changes you made.

degami’s picture

degami CreditAttribution: degami commented
Status: Needs work » Needs review

I will not even talk about it

hwi2’s picture

hwi2 CreditAttribution: hwi2 commented

Hey @degami, hang in there. Like geekygnr said, the review process can be daunting having others critique your work, but just remember, they ALL went through it. I am going through it right now with my first contributed module. It happens to the best of us. You are in good company. Ok, with that said, let's see if we can help you get your module approved by the community.

If your module is to become a full project, the one viewpoint is to also see it from the non-programmers. You know those Drupal admins who are just admins or end users, but not programmers.

Configuration Screen Works Well. Great job!

Your configuration screen at admin/config/media/page_background is well-done. It is very easy to configure, but it went downhill for me from there.

Issues with this module:

When I think of a page background, I think of the entire background being covered in the browser. If the background is not a full page width and height, then it needs to tile. When I add my image with a path, it only shows in the upper left center, but it is only an image that is 200 x 150 pixels. It needs to tile. For some reason, it does not.

I saw in your comment #8 above that you add the background to a region called "page_top". Why is it using a region at all? If the module is called "Page Background", then it should be attached to the entire page. In CSS, it should be applied to the BODY selector to accomplish the entire page. This will confuse a lot of page admins that are not techies or programmers. That needs to be fixed if I am to approve it for the community.

Looking through the .module, I see all of these CSS classes and selectors that you target with several drupal_add_css() functions like page_background_container.

I think a module like this needs to be simpler. A single hook_css_alter() would do the trick which would include your slider from your jQuery.

Also, in comment #8, you said does your theme render the "page_top" region? As programmers, we cannot make these assumptions that anyone will have a region with this name in their theme, but if you insist on using a region, then you need to create it programmatically with hook_system_info_alter(). 

/*
 * Implements hook_system_info_alter().
 */
function  page_background_system_info_alter(&$info, $file, $type) {
  if ($type == 'theme') {
    $info['regions']['page_top'] = t('Page Top');
	cache_clear_all();
  }
}

If you add this hook, your readme.md file needs to tell people to add the code to render the block in their theme as well.

Bruce

degami’s picture

degami CreditAttribution: degami commented

Hi Bruce,

thanks for your review, but:

the module was not meant to tile images in case of multiple images on the same path, instead it renders a slideshow (if jquery.cycle is installed) and it let you choose to have a slideshow or show a random image in case of no images set on specific path. It's up to you to add images with the right dimensions.

about the page_top region, you should check the default html.tpl.php specifications: https://api.drupal.org/api/drupal/modules%21system%21html.tpl.php/7

and the defined variables:

$page_top: Initial markup from any modules that have altered the page. This variable should always be output first, before all other dynamic content.
$page: The rendered page content.
$page_bottom: Final closing markup from any modules that have altered the page. This variable should always be output last, after all other dynamic content.

So if it is not rendered, maybe it's your theme that does not conform to the defaults.....

so i think implementing hook_system_info_alter() won't be useful

regards

alokvermaei’s picture

alokvermaei CreditAttribution: alokvermaei commented
FileSize
page background 4.png88.79 KB
page background 6.png98.44 KB
Page background.png108.17 KB
page background1.png93.26 KB
page backgroung 2.png89.07 KB

Hi Bruce,
Thanks for such awesome module . I have reviewed it and found some bugs .Please have a look into into it.I am also attaching the screenshot so that it will be more clear to you.

1. After module installation , I click on configure but it was not configured correctly . Refer screen shot 1.
2.Under page background configuration option ,under filter filed set there is no validation . Refer screen shot 2.
3.Filter click is not working after submit the form without any values. Refer screen shot 3.
4.On page background setting it's showing repetitive message.Refer screen shot 4.
5.Under filter tab it should not accept null value as well as the filed set should not be closed.Refer screen shot 5.

I am further reviewing the module and will give some suggestion on code level also whenever it will be completed.

Thanks

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha commented
Issue summary: View changes

updated issue summary. Added automated review link.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha commented
Issue summary: View changes
naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha commented
Status: Needs review » Needs work

Hi @degami,
Thanks for your contribution nice module !
There are some other modules (Background Images, Dynamic Background and Responsive Background Images) which provide similar type of functionality as this module.
Would you please specify the difference as the mentioned modules. Also it would be nice if you will add similar modules section in your issue summary and list the differences between your module and other modules.It would be also nice if you would mention it on your project page too.
I have updated issue summary and added the automated review link and made fewer changes so that it will looks good & formatted.

Thanks
Again!

degami’s picture

degami CreditAttribution: degami commented

Updated the configuration link in the .info file.
Removed the missing cycle warning in the configuration form, as it is now reported via hook_requirements in the status report page. Filter works for me. As it is a get form the validate is not working, so in the listing page, if filter is empty it simply doesn't filter.
About the order modules: there are many ways to accomplish a task. Mine is mine.

degami’s picture

degami CreditAttribution: degami commented
Status: Needs work » Needs review
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Postponed (maintainer needs more info)

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Having yet another module for the same purpose on drupal.org will not help users when choosing an appropriate module. Please join forces with one of the existing modules or post the differences to all the mentioned existing projects on your project page.

degami’s picture

degami CreditAttribution: degami commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)