Description

Once unit testing for each file of the component reaches 80% LOC coverage and there exists satisfactory DocBook documentation for the APIs, the Zend_Service_Technorati component needs to be promoted to core (i.e., moved to trunk with unit tests and documentation integrated).

Comments

Posted by Simone Carletti (weppos) on 2007-12-23T19:32:34.000+0000

r7247 includes 100% of Technorati API support and 90% of unit tests.

Next step is to reach 99% of unit tests.
It shouldn't be hard, all classes have been stressed by custom tests thus I only need to validate them, no more changes should be required.

Before starting the documentation it would be great to receive an overall feedback about the final product. :)

Posted by Simone Carletti (weppos) on 2007-12-25T17:31:42.000+0000

@ Darby

With r7263, I feel the library is ready.
I've included 99% of unit tests and full documentation.

What do you think? :)

Posted by Darby Felton (darby) on 2007-12-26T07:41:13.000+0000

Great! I'll review this today and post again here. Thanks for these contributions! :)

Posted by Darby Felton (darby) on 2007-12-27T16:05:55.000+0000

Sorry, Simone, for the delay in getting back with you... I haven't forgotten! :) I plan to take a look first thing tomorrow and post again here.

Posted by Darby Felton (darby) on 2007-12-28T08:23:31.000+0000

I get a fatal error when running the unit tests on r7284 with PHP 5.1.4 (WinXP):

As you can see, I test whether the method returns an exception if the constructor is called with an invalid argument.
The error should be catched by try/catch block, and in fact it is in my environment.

Do you have an idea why PHP 5.1.4 isn't aware of try/catch block here?

Posted by Darby Felton (darby) on 2007-12-28T09:33:33.000+0000

I haven't pored over the code yet, but it seems that this fatal error is not the result of an uncaught exception. Instead, a failure to meet a type hint in PHP < 5.2.0, I believe, results in a non-catchable fatal error.

Posted by Simone Carletti (weppos) on 2007-12-28T09:40:39.000+0000

The only alternative I can see right now is to skip all these tests whether PHP < 5.2.0.
This one is not the only test passing an invalid constructor but almost each class is tested against an invalid call.

What do you think?

Posted by Darby Felton (darby) on 2007-12-28T09:44:52.000+0000

Yes, this is what was done with other unit tests of the same type (e.g., see {{Zend_Db_Adapter_StaticTest}} and look for {{PHP_VERSION}}).

Posted by Darby Felton (darby) on 2007-12-28T09:59:28.000+0000

Now that I look into it more, it seems that I have based my assertion that type hinting failure results in non-catchable fatal errors on another person's incomplete work. I recommend that if you want to test failure to satisfy a type hint, try using a custom error handler.

Posted by Darby Felton (darby) on 2007-12-28T10:16:43.000+0000

Okay, maybe I should not have second-guessed myself. :) Or am I now doubting my doubts? ;) I ran the following script on my machine:

This is true right now, but according to Shahar's Zend_Uri improvements proposal Zend_Uri is going to support additional schemas.
Even if this is just a proposal, is reasonable to suppose that the future of Zend_Uri is open to new schemas thus the second check was created to be sure the result of

```

is a valid instance of a Zend_Uri_Http and not, for example, Zend_Uri_Mailto.
Unit tests are good enough to point out the error as soon as Zend_Uri will support more protocols, but restoring the control will prevent a 'last minute fix'.
What do you think?

About @todos you are right, there's an high number of @todo but just because I usually first develop the basic feature and I like to add notes to remember the feature might be improved in a second time.
I mean, @todos don't mean the code doesn't work, only it could have a better interaction with the environment (for example, @todo Zend_Uri_Http). ;)

I'm going to review the code addressing first all todos that may fit the classification 'should have', then I will create a new issue for any 'nice to have' as I did, for example, for ZF-2350.

bq. Just a final question: should Zend_Service_Technorati_ResultSet#getXML() be renamed to Zend_Service_Technorati_ResultSet#getXml(), isn't it?

Yes, it should be {{getXml()}} according to the naming conventions in the coding standards.

I'll perform what I hope might be a final review as soon as possible. I also suggest contacting the fw-webservices mailing list (and forwarding to fw-general, perhaps) to give the code a whirl to test it and provide feedback, to catch any lingering API issues, etc. that should be addressed prior to release with the framework core.

Posted by Simone Carletti (weppos) on 2008-01-02T17:22:56.000+0000

Fixes committed in r7333.

Posted by Simone Carletti (weppos) on 2008-01-15T02:37:49.000+0000

Hi Darby,
I was wondering if you are going to move the component to core before the upcoming code freeze. :)

Posted by Darby Felton (darby) on 2008-01-29T15:46:52.000+0000

Attached test output from HEAD of the {{trunk/}} (PHP 5.1.4, WinXP)

Posted by Simone Carletti (weppos) on 2008-01-30T02:29:29.000+0000

Gasp, I'm absolutely shocked by the output.
It seems something goes wrong with Zend_Date, I will investigate.

Posted by Simone Carletti (weppos) on 2008-01-30T02:52:36.000+0000

Hi Darby,
I investigated the issue and, as I supposed, it's related to Zend_Date.
It seems Zend_Date no longer recognizes valid dates (ZF-2524).

I'm waiting for a Thomas's answer to check what is the origin of the issue.
The last time I run the suite (02/Jan/08), all tests passed without any problem.

Posted by Darby Felton (darby) on 2008-01-30T13:28:27.000+0000

Yes, it seems as though something has changed with Zend_Date, as I have never seen such errors in the Zend_Service_Technorati tests until now.

Probably it would be a simple matter to provide the date format, as Thomas suggests, to get the tests passing again.

I am concerned, however, that there has been a backward compatibility break with Zend_Date, inasmuch as people have been depending on certain behavior that is now broken. :( That said, perhaps it is not a backward compatibility break in the sense that perhaps the usage within Zend_Service_Technorati was broken, non-standard, not documented, or otherwise unsupported behavior. Thomas could probably shed some light on this. All things considered, I don't really understand why {{Zend_Date::isDate()}} cannot grok the same date value that {{new Zend_Date()}} supports. Strange, indeed.

Posted by Simone Carletti (weppos) on 2008-01-30T16:15:24.000+0000

I found the guilty, the change that broke BC compatibility (at least in Zend_Service_Technorati case).
I'm waiting for Thomas answer.

It would be hard to provide date format because Technorati is not so coherent!
You should consider that they created an own DTD but the most part of API XML responses is invalid against their own DTD!

Posted by Simone Carletti (weppos) on 2008-02-01T04:35:49.000+0000

I fixed the issue with r7751 using strtotime to validate the input.

Posted by Darby Felton (darby) on 2008-02-01T10:21:34.000+0000

Hi Simone,

It looks as though Zend_Service_Technorati is ready to be moved to core. Are you aware of anything that should be done beforehand?

Would you like to migrate the component, or would you like me to do it on your behalf?

Thanks for these excellent contributions! :)

Posted by Simone Carletti (weppos) on 2008-02-01T10:30:24.000+0000

Hi Darby,
according to official documentation only a core contributor could promote a component to core this is why I never moved a single bit. ;)

By the way, if you want I can move it and left you free for other activities.
I suppose I only need to
* svn move incubator/library incubator/test to /library /test
* promote documentation
* update docbook single files and main index.
I guess there's nothing more to do, isn't it?

Posted by Darby Felton (darby) on 2008-02-01T11:32:43.000+0000

Sorry, Simone, I didn't realize that it was documented that only a Zender could perform the actual promotion. I have confidence that you would have no problem doing it, but in the interest of following the rules, I'll go ahead and make promote it today as soon as possible. :)