Archive module: fapi conversion + bug fixes [#52418]

Attached patch:

Converts the archive module form to the fapi model.
Adds 'No posts found.' message if no posts are available. Perhaps the two instances of the status message can be combined together, but I've left if as it is for the moment. I think it'd be useful if the search module had a 'search by date' facility that the archive module could refer to at this point.
Adds some styling to provide a margin to both the form and the error message.
Removes unused variable $original from archive_calendar
Removes the section of code from archive_calendar that uses year, month and day in $_POST information as input. I don't see what the point of this is and don't believe it's used by any other module either.. Rather odd..
Moves hook_menu up.

That's about it.

Please review. Thanks :)

-K

Comment	File	Size	Author
#9	archive_3.patch	7.12 KB	Zen
#9
#8	archive_2.patch	7.12 KB	Zen
#8
#6	eall.php.txt	294 bytes	Zen
	archive_1.patch	7.1 KB	Zen

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dries CreditAttribution: Dries commented 4 March 2006 at 18:18

This is likely to generate warnings. You have to put quotes around string values:

+function archive_browse_form_submit($form_id, $form_values) {
+  return "archive/$form_values[year]/$form_values[month]/$form_values[day]";
+}

Comment #2

Zen CreditAttribution: Zen commented 4 March 2006 at 18:30

That's the original code untouched and I think it's correct - quotes shouldn't be used around associative array keys when inside a double quoted string. Or if you want to use them then the variable should be inside curly brackets (IIRC anyways).

-K

Comment #3

Zen CreditAttribution: Zen commented 4 March 2006 at 18:34

Couldn't find an exact reference to this on php.net , but you can see an example on phpdig.net : http://www.phpdig.net/ref/rn03.html [third snippet from the top].

Thanks.
-K

Comment #4

chx CreditAttribution: chx commented 4 March 2006 at 18:53

archive/$form_values[year]/$form_values[month]/$form_values[day] incorrect.
'archive/' . $form_values['year'] etc. if php docs says otherwise, then those examples are sloppy code. flip on E_ALL and watch for yourself.

It's the original code? So what?

Comment #5

Zen CreditAttribution: Zen commented 4 March 2006 at 19:13

chx: It doesn't apply within a double-quoted string is what I'm saying. Array keys need to be quoted everywhere else except inside a double quoted string.

"It's the original code? So what?" - So we would have possibly encountered this before?

I'll test this shortly..
-K

Comment #6

Zen CreditAttribution: Zen commented 4 March 2006 at 19:22

File	Size
eall.php.txt	294 bytes

Please check the attached test script.

-K

Comment #7

Crell CreditAttribution: Crell commented 4 March 2006 at 19:51

Drupal's the only code I write these days not under E_ALL. :-)

I've not heard of echo "$form[a]" working before. It may, but it's ugly and unclear.

Personally, I always use {} if I'm putting a variable inside a double-quoted string. It's not always necessary but a good habit to get into, because it often is, especially when you're dealing with arrays so often.

So the correct way to do that function, IMHO, would be:

+function archive_browse_form_submit($form_id, $form_values) {
+ return "archive/{$form_values['year']}/{$form_values['month']}/{$form_values['day']}";
+}

It's E_ALL compliant, it's readable, it's a common style.

Comment #8

Zen CreditAttribution: Zen commented 4 March 2006 at 20:09

File	Size
archive_2.patch	7.12 KB

OK we're splitting hairs here now. Patch 1 attached.

-K
P.S And I hate curly braces as they are ugly and unclear ;)

Comment #9

Zen CreditAttribution: Zen commented 4 March 2006 at 20:13

File	Size
archive_3.patch	7.12 KB

Comment #10

Zen CreditAttribution: Zen commented 4 March 2006 at 20:58

http://us2.php.net/language.types.array <-- It clearly states here that "$form[year]" is perfectly valid. So, please use patch number 1.

Thanks
-K

Comment #11

Zen CreditAttribution: Zen commented 4 March 2006 at 21:00

Filter ate the rest of my message:

The above php.net link clearly states that using "$form[year]" is perfectly valid and acceptable. So please use patch no. 1.

Thanks
-K

Comment #12

chx CreditAttribution: chx commented 4 March 2006 at 21:57

Is this some form of stupidity contest?

Again. Slowly. Drupal uses

'archive/'. $form['year']. '....

what's in this that can not be understood? There is a string constant, a concatenation operator, a variable and so on. String constants -- even if they happen to be in array indexes -- are to be put in single quotes. You have more than 30K LoC to learn all this.

Comment #13

Zen CreditAttribution: Zen commented 4 March 2006 at 22:33

I don't particularly care.. I'm fine with all three versions of this patch. Dries and yourself (chx) said that this would cause an error with E_ALL.. I've supplied PHP documentation and a test script that shows it doesn't and I've also supplied alternate versions of the patch to satisfy all parties. Nothing more that I can do.

And for the record, the syntax that appears to have gotten under chx's skin is used elsewhere in core as well.. Finding it and patching it.. I leave to chx as an exercise :)

-K