Michael Gauthier [2010-11-16 15:36 UTC] Package source file is a 404 so I cannot review.
A high-quality XMPP library would be totally awesome for PEAR.
Quickly looking at the documentation the first change required for PEAR acceptance is to make the file structure conform to PEAR's file structure requirements.
The second is to use class constants instead of global constants.
Abhinav Singh [2010-11-16 21:13 UTC] Hi Michael,
Not sure why a 404 is returned for the pkg download link, shall update it. Kindly download the source code from here: https://github.com/abhinavsingh/JAXL
Will also look at PEAR code styling and see if anything what can be done to make Jaxl PEAR compatible.
Till Klampaeckel [2010-11-18 16:58 UTC] I just saw this proposal just now -- very cool stuff and certainly a great addition to PEAR. :-)
Mike mentioned the preliminary things, let us know when they are done so we can review more?
Ken Guest [2010-11-18 18:16 UTC] What Till said is an understatement at the very least...this is extremely cool and I can't wait to use it.
Christian Weiske [2010-12-22 11:00 UTC] Any news here?
Abhinav Singh [2010-12-27 11:08 UTC] Hi,
I have made a few changes to the library as required by PEAR coding standard but my biggest fear still remains with Jaxl library file and class naming convention. The folder structure looks like:
- /core
-- jaxl.class.php
-- jaxl.something.php
- /xmpp
-- xmpp.class.php
-- xmpp.get.php
-- xmpp.something.php
- /xep
-- jaxl.0045.php
-- jaxl.0060.php
- /env
-- jaxl.ini
-- jaxl.js
Applications should simply do:
require_once '/path/to/jaxl/core/jaxl.class.php';
Jaxl class have an internal class loader method which automatically includes other library folder files into the environment.
The class names are like:
JAXL, JAXLUtil, XMPP, XMPPGet, JAXL0045, JAXL0060 etc which is again something not as per pear coding standard.
Changing file names and internal class names will be a big big change for library and it's existing users. Is there a workaround so that file names and class names can remains as it is, and Jaxl library can be submitted to pear repository. Ofcourse, other pear coding standards will be followed as defined.
Brett Bieber [2010-12-27 15:10 UTC] I think following the class name and file naming conventions is a basic requirement, and all PEAR packages should be PSR-0 compliant. The PEAR standards require this, and the PHP standards group has adopted this same file naming structure. http://groups.google.com/group/php-standards/web/psr-0-final-proposal
Christian Weiske [2010-12-27 17:11 UTC] > Changing file names and internal class names will be a big big change
> for library and it's existing users. Is there a workaround so that
> file names and class names can remains as it is,
You have to follow the PEAR class and filename conventions.
However, you could distribute a "BC" folder that contains the old file and class names, extending the new classes.
Abhinav Singh [2010-12-31 09:41 UTC] @Brett
My bad that i didn't take a look at pear coding standards 2 yr back when i started on the library. However in it's current state changing all file and class names will be a huge task and inconvenience for it's existing users.
@Christian
I have a few doubts:
What exactly is a "BC" folder? As i understand pear package will only contain dummy files with appropriate names and class names. However users will have to download this "BC" folder from else to get the library working? Or will this "BC" folder too be a part of downloaded pear package?
Christian Weiske [2010-12-31 10:29 UTC] BC = backwards compatibility
You can include that "compatibility" folder with your pear package, that's no problem.
I suggest giving it - and its files - the "data" role to not tainting the include path with old style files:
Example:
file data/compatibility/JAXL0045.php
<?php class JAXL0045 extends Net_XMPP_XEP_0045 {} ?>
Users would require_once the
> /path/to/pear/data/net_xmpp/compatibility/jaxl.class.php
As you see from the class name, I also suggest that you name the package Net_XMPP - it implements a network protocol, XMPP - all packages in PEAR currently have that naming scheme. Does that seem ok for you?
Abhinav Singh [2011-01-04 15:34 UTC] Christian this ofcourse seems like a good solution. Here are what i understand the procedure will be.
1) I understand that downloaded pear package will be installed under folder
/path/to/pear/net_xmpp/
2) Following 4 folders will be found inside:
/path/to/pear/net_xmpp/xep/
Contains dummy files like 0045.php, 0060.php etc
/path/to/pear/net_xmpp/core/
Contains dummy files like util.php, parser.php etc
/path/to/pear/net_xmpp/xmpp/
Contains dummy files like get.php, send.php etc
/path/to/pear/net_xmpp/jaxl/
Contains the compatibility files as found in current public release of jaxl library
PS: By dummy files we mean they only have something class declaration like:
class NET_XMPP_XEP_0045 {}
3) Each class inside compatibility folder will extend dummy file classes e.g.
class JAXL extends NET_XMPP_CORE_JAXL.php {
// contains original library source code compatible with PEAR coding standards
}
4) Finally users can write xmpp apps just by doing:
require_once '/path/to/pear/net_xmpp/jaxl/jaxl.class.php
Kindly let me know and i will be happy to have something like this ready.
Christian Weiske [2011-01-04 15:50 UTC] Not completely.
First, please remember that _ in class names get converted to directories in the file name according to the PEAR coding standards: A class "Console_CommandLine_Option" has the file name "Console/CommandLine/Option.php" and gets installed into "/path/to/pear-config-php_dir/Console/CommandLine/Option.php".
Second, the new classes are the one with code, not the compabitility ones.
Example:
- The old class name "JAXL0045" will get renamed to Net_XMPP_XEP_45 (or _0045)
- The file will be"/path/to/pear-config-php_dir/Net/XMPP/XEP/45.php"
- To provide backwards compatibility with the old jaxl class style, a dummy class JAXL0045 exists that extends Net_XMPP_XEP_45 and gets installed in /path/to/pear-config-data_dir/jaxl/compatibility/xep/jaxl.0045.php
- Users are encouraged to use the new style classes, but may use the compatibility classes for old projects
I'll try to fork your github project and make some exemplary changes so you can understand my comments better within two days.
Christian Weiske [2011-01-04 21:02 UTC] I opened a "pearified" branch of my jaxl fork on github. It should serve as example how to do it, and could server as starting point for you to get jaxl prepared for PEAR:
https://github.com/cweiske/JAXL/tree/pearified
Please have a look at it.
- the compat directory is not full yet, I only added some examples so you can see how it could look like
- I would split the package up into two packages: Net_XMPP and Net_XMPP_Client. Net_XMPP only implements the xmpp protocol details, while Net_XMPP_Client contains all the files that assists you in creating the client.
Some things I saw when looking through the source code:
- Log -> replace with Log package
- no static logging, always use configured instance
- util -> replace curl() with http_request2 usage
- XEP classes without leading zeros, since 4 zeros may not be enough in the future
- get rid of jaxl_require
- replace define() with class constants
- rename jaxl in the files to net_xmpp
- follow pear coding standards, run phpcs on it
- php5: public/private/protected instead of "var"
- use type hints in method parameters
- get rid of JAXL_BASE_PATH and jaxl_instance_count - no global constants, no global variables allowed!
- I only fixed the file names and the "class $classname" declarations. I did not test them, and did not rename class usages - this is left to do
- there are absolutely no unit tests, which is a shame for such a big bag of source code
- docblocks everywhere are missing. very important is the description of array keys that are required in the parameter arrays, i.e. in Net_XMPP_Get
- return values are also not documented
- parts of Net_XMPP_Client_Util probably can be replaced by using pear packages, i.e. isWin() is available in the PEAR System class, hmacMD5 is in the PEAR Crypt_HMAC package
Christian Weiske [2011-01-15 18:45 UTC] Hey Abhinav, any updates on the code?
Abhinav Singh [2011-01-19 15:37 UTC] Hi Christian,
I went through the fork created by you and steps recommended in your comments. It seems like before Jaxl can get a place in pear repository, i will need to brush up quite a few things. I currently have little bandwidth on my side to get this done, but will come back to it sometime again and see what all can be done.
Regards,
Abhinav
|
