Bug 505356 - Review Request: php-PHPMailer - PHP email transport class with a lot of features
Review Request: php-PHPMailer - PHP email transport class with a lot of features
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 508817
  Show dependency treegraph
 
Reported: 2009-06-11 12:00 EDT by Patrick Monnerat
Modified: 2013-01-28 15:44 EST (History)
6 users (show)

See Also:
Fixed In Version: 5.0.2-3.el5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-07 00:53:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
fedora: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Patrick Monnerat 2009-06-11 12:00:08 EDT
Spec URL: http://monnerat.fedorapeople.org/php-PHPMailer.spec
SRPM URL: http://monnerat.fedorapeople.org/php-PHPMailer-5.0.2-1.fc12.src.rpm

Description: PHP email transport class featuring file attachments, SMTP servers, CCs, BCCs, HTML messages, word wrap, and more.
Sends email via sendmail, PHP mail(), QMail, or with SMTP.

rpmlint output: none
koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1405528
Comment 1 Remi Collet 2009-06-18 14:21:24 EDT
I don't think installing class directly under /usr/share/php is a good idea
(if lot of package does this, we are going to create a lot of conflicts)

Should use sed rather than ed

sed -i -e ...

I think a better solution is to install in /usr/share/php/PHPMailer and Set PluginDir in class (quite strange than SetLanguage doesn't use it)

Tagging lang file seems usefull, something like 

# Lang
for i in %{buildroot}%{_datadir}/path/to/language/*.php
do
  lang=$(basename $i)
  echo "%lang(${lang:15:2}) %{_datadir}/path/to/language/${lang}"
done >%{name}.lang

And 
%file -f %{name}.lang
Comment 2 Patrick Monnerat 2009-06-19 04:28:50 EDT
> I don't think installing class directly under /usr/share/php is a good idea

I totally agree. But I just follow the packaging guidelines: https://fedoraproject.org/wiki/Packaging:PHP#File_Placement
Comment 3 Patrick Monnerat 2009-07-07 09:37:37 EDT
New version: http://monnerat.fedorapeople.org/php-PHPMailer-5.0.2-2.fc10.src.rpm
* Fri Jun 19 2009 Patrick Monnerat <pm@datasphere.ch> 5.0.2-2
- Suppress "ed" build requirement.
- Tag language files.
- Move class files to a package-specific directory.

rpmlint output: none
koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1458803
Comment 4 Remi Collet 2009-07-18 07:49:25 EDT
PHP CompatInfo report (not fully reliable info, but give some usefull ones): 
$ pci -d /usr/share/php/PHPMailer/ -p -S
+-----------------------------+---------+---+------------+--------------------+
| Files                       | Version | C | Extensions | Constants/Tokens   |
+-----------------------------+---------+---+------------+--------------------+
| /usr/share/php/PHPMailer//* | 5.1.0   | 1 | date       | catch              |
|                             |         |   | filter     | private            |
|                             |         |   | hash       | protected          |
|                             |         |   | mbstring   | public             |
|                             |         |   | openssl    | throw              |
|                             |         |   | pcre       | try                |
+-----------------------------+---------+---+------------+--------------------+


mbstring seems not mandatory (ready quickly the code), but really usefull.

Requiring "php" brings a lot of dependencies (mainly httpd) which are not need (could use this without apache: with another web server, in cgi, or from command line)


So Requires seems to be :
Requires:	php-mbstring >= 5.1.0

This is only a quick note, I will do the full review ASAP.
Comment 5 Remi Collet 2009-07-18 08:43:07 EDT
URL seems to have change
http://phpmailer.worxware.com/

+
Comment 6 Gianluca Sforna 2009-07-18 09:07:26 EDT
I'll let mantis Require this when it lands in the repo, drop me a note if I can do anything to help here.
Comment 7 Patrick Monnerat 2009-07-20 08:43:39 EDT
The URL change (redirection) is very new: next RPM will be updated accordingly.
About the "Require" comment: I agree with you, but no usable php (either php or php-cli) will be brought in if you "yum install php-PHPMailer". Only php-common. The best solution would be something like "Provide: php-runtime" in php and php-cli, that can be "Require"d in other modules (a bit like "webserver" in apache and other http servers).
Thanks for you comments above. I will wait your review before issuing the next release.
Comment 8 Jason Tibbitts 2009-07-31 23:31:40 EDT
Setting the fedora-review flag as that seems to have been overlooked.
Comment 9 Remi Collet 2009-08-01 03:16:29 EDT
@Gianluca can you try mantis with this package ? (glpi works fine with it)

@Patrick

First, sorry for the long delay..

> "Provide: php-runtime"

I really think than requiring php-common is enough for most libraires. This package will be required by web apps (mantis, glpi, ...) which will requires more stuff (apache, ...) if needed.

Requiring too much things is more an issue than not requiring such "trivial" and optional package. And a lot of users want to use lighthttp rather than apache.

I think description can be improved, for example with the upstream one
------
Full Featured Email Transfer Class for PHP. PHPMailer features:

    * Supports emails digitally signed with S/MIME encryption!
    * Supports emails with multiple TOs, CCs, BCCs and REPLY-TOs
    * Works on any platform.
    * Supports Text & HTML emails.
    * Embedded image support.
    * Multipart/alternative emails for mail clients that do not read HTML email.
    * Flexible debugging.
    * Custom mail headers.
    * Redundant SMTP servers.
    * Support for 8bit, base64, binary, and quoted-printable encoding.
    * Word wrap.
    * Multiple fs, string, and binary attachments (those from database, string, etc).
    * SMTP authentication.
    * Tested on multiple SMTP servers: Sendmail, qmail, Postfix, Gmail, Imail, Exchange, etc.
    * Good documentation, many examples included in download.
    * It's swift, small, and simple.
------


REVIEW:
+ rpmlint is silent
php-PHPMailer.src: I: checking
php-PHPMailer.noarch: I: checking
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
+ package name ok
+ spec file name ok
+ package must meet the  Packaging Guidelines
+ Fedora approved license (LGPLv2+)
+ match the actual license
+ license included
+ spec file written in American English and legible (well don't think #---- are usefull)
+ sources match the upstream 
2f7296bb63e863c5528c2d591e38f4e5  PHPMailer_v5.0.2.tar.gz
+ rpmbuild ok (F11.x86_64)
+ mock ok (F11.i386)
+ BuildRequires ok
- Requires (see above)
php >= 5.0.0
+ locales handled properly
+ no .so
+ own all directories that it creates
+ %defattr ok
+ %clean ok
+ %install start with rm -rf...
+ consistently use macro
+ contain code
+ no large documentation and not affect the runtime of the application
+ no gui
+ not own files or directories already owned by other packages
+ valid UTF-8
+ install and works finr (tested with glpi)

Should fix URL and description
Must fix php-mbstring dependency
Comment 10 Patrick Monnerat 2009-08-03 09:00:45 EDT
Many thanks for the review, Remi.

New SRPM: http://monnerat.fedorapeople.org/php-PHPMailer-5.0.2-3.fc10.src.rpm
Fixes URL, description and dependency
rpmlint silent
koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1575575
Comment 11 Remi Collet 2009-08-03 12:29:09 EDT
+ Home page change : ok
+ Package description from new home page. : OK
+ Requires php-mbstring : OK


APPROVED
Comment 12 Patrick Monnerat 2009-08-04 04:45:13 EDT
Thanks for your help, Remi.

New Package CVS Request
=======================
Package Name: php-PHPMailer 
Short Description: PHP email transport class with a lot of features
Owners: monnerat
Branches: F-10 F-11
InitialCC:

Many thanks in advance for CVS action
Comment 13 Jason Tibbitts 2009-08-04 16:29:55 EDT
CVS done.
Comment 14 Remi Collet 2009-08-04 16:42:06 EDT
@Patrick

What about also maintaining this package in EPEL-5 ?
Comment 15 Patrick Monnerat 2009-08-05 04:32:17 EDT
> What about also maintaining this package in EPEL-5 ?

I'm not interested in doing so. But please, feel free to take this task if it is valuable for you.
Comment 16 Fedora Update System 2009-08-05 05:59:02 EDT
php-PHPMailer-5.0.2-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/php-PHPMailer-5.0.2-3.fc10
Comment 17 Fedora Update System 2009-08-05 05:59:08 EDT
php-PHPMailer-5.0.2-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/php-PHPMailer-5.0.2-3.fc11
Comment 18 Remi Collet 2009-08-05 12:10:39 EDT
@Patrick : yes I need it, so I ask the new branch with me as owner.

Package Change Request
======================
Package Name: php-PHPMailer 
New Branches: EL-5
Owners: remi  (only for this new branch)
Comment 19 Patrick Monnerat 2009-08-05 12:18:32 EDT
> I need it, so I ask the new branch with me as owner.
You're welcome !
Comment 20 Jason Tibbitts 2009-08-05 12:49:40 EDT
CVS done.
Comment 21 Fedora Update System 2009-08-07 00:53:48 EDT
php-PHPMailer-5.0.2-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 22 Fedora Update System 2009-08-07 00:54:13 EDT
php-PHPMailer-5.0.2-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 23 Fedora Update System 2009-08-08 01:35:05 EDT
php-PHPMailer-5.0.2-3.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/php-PHPMailer-5.0.2-3.el5
Comment 24 Fedora Update System 2009-08-31 18:54:01 EDT
php-PHPMailer-5.0.2-3.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.