I'm suprised no one else has mentioned this. I setup a block in Views and set ddblock to use it. When the view loads I also get loads of PHP notices popping up in drupals message system. Obviously, this is not a huge issue as a production environment wouldn't have such a setting on but it is really annoying whilst developing and I do use PHP srtict and avoid trying to use undefined variables. most of the undefined variables occur when trying to set $configuration_settings. This is also and interesting area because I noticed you're building an array and serializing it before using variable_set. You can do this:

$settings = array(
    'key' => 'value',
    'foo' => 'bar',
);
variable_set('ddblock_block_'. $origin .'_'. $delta .'_cycle_settings', $settings);

It isn't necessary to serialize the information yourself.

It doesn't seem to me that all these lines are needed to set $configuration_settings. Why not set a base array with all the possible keys for the configuration then merge it with $edit:

$settings = ddblock_load_settings($origin, $delta);
$settings += $edit;
variable_set('ddblock_block_'. $origin .'_'. $delta .'_cycle_settings', $settings);

anyway, can we do something about the undefined variables?

CommentFileSizeAuthor
#2 ddblock.patch21.73 KBjosh waihi
#2 ddblock.class_.php.txt4.16 KBjosh waihi

Comments

ppblaauw’s picture

Status: Active » Postponed (maintainer needs more info)

Traveled this week from the Netherlands to the Philippines. Was a busy time for me.

Thanks for reporting.

Yes, I know it's not the most elegant code.
Just the remains from the code we used in the beginning when the module was just a test to develop a block module.

To improve the code I have some questions:

I just could get serialize and unserialize in the code and it works ok?
What would the code look like for the ddblock_load_settings function you suggest?
How do you control that only integer values are inserted for some fields?

Any other suggestions for code improvement?

josh waihi’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new4.16 KB
new21.73 KB

I've coverted $configuration_settings into a configuration class that predefines every variable in a single place, this makes initializing all the vairables way easy and setting all the variables really easy too. The current code seems to a lot of:

$myVar = $other_var['myVar'];

which is stupid case you then have to manage lots of string variables rather than a single collection on variables in an array. It makes thing harder to debug to. Reminds me of PHP 3. Speaking of which, my implementation will only work in PHP 5. I hope thats ok. it would be possible to back port.

This patch will need review as it does modify some behaviour, and it will need to be altered so that it doesn't.

Naturally the attach class file needs to be renamed to ddblock.class.php

asak’s picture

subscribing.

ppblaauw’s picture

Assigned: Unassigned » ppblaauw
Status: Needs review » Active

Tested the patch code.
This will go into the next version of the ddblock module.

Thanks Josh for your time and effort.