JUtility class -- why does it even exist?

[Paladin]

I'm almost sorry to be so blunt about it, but this is a particularly
egregious example of class abuse. Let's take a walk through it, shall
we, while I ask the impertinent questions:

sendmail, sendAdminMail: There's already a perfectly fine JMail class,
and these functions are interfaces to it. They belong as member
methods of JMail, they have no reason to exist apart from it. (If you
want to get *really* picky about it, every object that is allowed to
send mail should have a "sendmail" somewhere in its inheritance tree,
which would consist of a getMailer call to JFactory and a send, just
two lines of code -- or even a single line like JFactory->getMailer
().send( arglist). Everything else belongs in JMail.)

getHash: an MD5 of a config value. Belongs either in the interface for
the config values (JRegistry) or the class that value is used. (see
next)

getToken - simply a session hash, belongs in JSession. And while we're
on the subject, why two different anti-spoof hashes? If one is for
session use, and getHash() is for the rest of the app, shouldn't
getHash be in JApplication?

parseAttributes - "for XML style attributes" Belongs in simpleXML,
then, right? Or, if actually used in processing many types of
documents (as it actually seems to be) then it should be in the
inheritance tree for processing those documents. Either way, not here.

isWinOS - Don't have a single instance of it being called, but even if
so, shouldn't it be in JApplication? (and why PHP_OS rather than the
recommended php_uname('s')?)

dump - Another unused function. The fact it might be useful while
debugging implies it should be in a "debug" class that's only loaded
when debugging is on, and not in a class that's always loaded.

array_unshift_ref -- AFAICT, this is used only by stream to push a
resource on the top of a stack. If that's the case, move it to stream.
If not, create a data class for the data type that will require this
and put it there.

return_bytes -- AFAICT used only in InstallerModelWarnings. Move it
there. Or into an inheritance chain above there so it's available to
whatever else might be using it.

Recommendation for this class is to empty it completely of every
function Joomla plans on continuing to use, and leave only deprecated
functions and proxies for the moved functions, with an eye toward
eliminating the entire class in the future. Utility classes only serve
as cruft attractors, they serve no real purpose in an OO approach, and
generally only complicate matters both by encouraging more such
oddities in the code as well as by encouraging developers to duplicate
methods (writing a method in the class they're working on because it
belongs there, even if it exists in utilities).

If you're truly committed to keeping such things around, then drop the
pretense of Object Orientation for them, and simply make them
procedural functions , like jimport and jexit. It's counterproductive
and confusing to pretend JUtilities is actually a class.

[Andrew Eddie]

Agree in principle with everything.

2009/11/10 Paladin <arlen....@gmail.com>:

>
> I'm almost sorry to be so blunt about it, but this is a particularly
> egregious example of class abuse. Let's take a walk through it, shall
> we, while I ask the impertinent questions:
>
> sendmail, sendAdminMail: There's already a perfectly fine JMail class,
> and these functions are interfaces to it. They belong as member
> methods of JMail, they have no reason to exist apart from it. (If you
> want to get *really* picky about it, every object that is allowed to
> send mail should have a "sendmail" somewhere in its inheritance tree,
> which would consist of a getMailer call to JFactory and a send, just
> two lines of code -- or even a single line like JFactory->getMailer
> ().send( arglist). Everything else belongs in JMail.)

Yep.

>
> getHash: an MD5 of a config value. Belongs either in the interface for
> the config values (JRegistry) or the class that value is used. (see
> next)

Agree with moving this to JApplication::getHash

>
> getToken - simply a session hash, belongs in JSession. And while we're
> on the subject, why two different anti-spoof hashes? If one is for
> session use, and getHash() is for the rest of the app, shouldn't
> getHash be in JApplication?

Agree generally. I think there's possibly some merit in keeping the
clear token and then having one for usage in forms. So maybe
JUtility::getToken moves to JSession::getFormToken which is static.
Not 100% sure on that one myself other than it should change.

>
> parseAttributes - "for XML style attributes" Belongs in simpleXML,
> then, right? Or, if actually used in processing many types of
> documents (as it actually seems to be) then it should be in the
> inheritance tree for processing those documents. Either way, not here.

Actually it's for parsing a=1&b=2, etc. Probably something native we
can use or bolt it onto the JUri class.

>
> isWinOS - Don't have a single instance of it being called, but even if
> so, shouldn't it be in JApplication? (and why PHP_OS rather than the
> recommended php_uname('s')?)

Thought is was used in JFile or something. I think JApplication is
fine if it's necessary at all.

>
> dump - Another unused function. The fact it might be useful while
> debugging implies it should be in a "debug" class that's only loaded
> when debugging is on, and not in a class that's always loaded.

Nuke it.

>
> array_unshift_ref -- AFAICT, this is used only by stream to push a
> resource on the top of a stack. If that's the case, move it to stream.
> If not, create a data class for the data type that will require this
> and put it there.

Yep, replace it with some real code in the right spot.

> return_bytes -- AFAICT used only in InstallerModelWarnings. Move it
> there. Or into an inheritance chain above there so it's available to
> whatever else might be using it.

Move as suggested.

>
> Recommendation for this class is to empty it completely of every
> function Joomla plans on continuing to use, and leave only deprecated
> functions and proxies for the moved functions, with an eye toward
> eliminating the entire class in the future.

Agree. Deprecate the whole thing.

Regards,
Andrew Eddie
http://www.theartofjoomla.com - the art of becoming a Joomla developer

[Louis Landry]

Hi Paladin,

You don't have to be sorry for being blunt.  So long as we can all keep our heads I think its fantastic that we can be open and ask direct questions of one another.

This class comes from the process of moving from Joomla 1.0 to 1.5.  Most of these if not all of these functions existed in some form within the Joomla 1.0 code base and we did not have a good place for them within the Joomla 1.5 Framework.

It is easy to forget that when we were designing this framework we had an existing application with millions of installed instances, thousands of extensions, and hundreds if not thousands of developers all based on a code base that was several years old.  We took that scenario and completely rewrote the entire foundation, APIs and structure while trying desperately to be as non-disruptive as we could to the ecosystem.  In some aspects we were very successful, in some we were not.  One of the byproducts of that process is that some functions are less than optimal and some compromises had to be made.  What you are seeing with this class, and probably a few other odds and ends within the framework is something like that.  Trying to make the transition as easy as possible for extensions and developers that are used to a certain set of functions.

Now that I am done with the blast from the past... on to your specific thoughts.

I don't agree with you on it never being appropriate to group static functions in a class, if I am reading you right.  I do think that this class in an ideal situation isn't necessary, and we can certainly deprecate it and remove it in the near future.  

I'm fine with all of your suggestions, and think that the parseXMLattributes method really sort of belongs in the JArrayHelper class ... maybe as a fromXMLattributes() method or something like that.  That's really what is going on with it.

- Louis

--
Development Coordinator
Joomla! ... because open source matters.
http://www.joomla.org

[Paladin]

On Nov 9, 9:23 pm, Louis Landry <louis.lan...@joomla.org> wrote:
> You don't have to be sorry for being blunt.  So long as we can all keep our
> heads I think its fantastic that we can be open and ask direct questions of
> one another.

Just don't be afraid to tell me I'm wrong, and we'll get along fine. ;
{>} I can handle either the idea that I may actually *be* wrong (it's
been known to happen) or the idea that the project team, for reasons
of its own, wants to continue in error, at least for a while longer. ;
{>}

While I don't expect to always get my way, I *do* expect to have my
say.

> I don't agree with you on it never being appropriate to group static
> functions in a class, if I am reading you right.  I do think that this class
> in an ideal situation isn't necessary, and we can certainly deprecate it and
> remove it in the near future.

I think you may have overstated me by just a little. I don't have a
real problem with static methods in a class, exactly. I have a real
problem with classes full of static methods being used as nothing more
than glorified function libraries. Classes are for data and the
methods that work with them.

Yes, in an ideal environment, I'm against any and all static methods.
But, since PHP isn't a real OO language, with real OO features, some
compromises must be made. But a class named "Utiltities" is prima
facie evidence that far too many compromises have been made. ;{>} Such
classes should at least *pretend* to be about logically connected data
and methods.

> I'm fine with all of your suggestions, and think that the parseXMLattributes
> method really sort of belongs in the JArrayHelper class ... maybe as a
> fromXMLattributes() method or something like that.  That's really what is
> going on with it.

Hmm. Interesting idea. Maybe that's where the tickling in the back of
my head was coming from; something struck me as familiar, and I'd just
been in JarrayHelper earlier this week.

On Mon, Nov 9, 2009 at 8:45 PM, Andrew Eddie <mambob...@gmail.com>
wrote:

> Agree generally.  I think there's possibly some merit in keeping the
> clear token and then having one for usage in forms.  So maybe
> JUtility::getToken moves to JSession::getFormToken which is static.
> Not 100% sure on that one myself other than it should change.

I'll drop it in a petrie dish and we'll see if anything grows.

For the rest, I'll let it germinate a bit and line up some more
concrete proposals for it, now I know it's a probable go.

[Andrew Eddie]

2009/11/10 Paladin <arlen....@gmail.com>:

>
> Just don't be afraid to tell me I'm wrong, and we'll get along fine. ;
> {>} I can handle either the idea that I may actually *be* wrong (it's
> been known to happen) or the idea that the project team, for reasons
> of its own, wants to continue in error, at least for a while longer. ;
> {>}
>
> While I don't expect to always get my way, I *do* expect to have my
> say.

I think we should mount that on a plaque at the NY Devcon and chant it
down the streets of Manhattan :) Love it!

[Hannes Papenberg]

Just a short note from me: If we move all these functions to another
class, don't keep an empty class with proxies, but remove the class in
1.6 directly. I'd vote for this. Anyone very strongly against this?
Otherwise: Could you create a patch? :-)

Hannes

Paladin schrieb:

[Ian MacLennan]

I don't really see a compelling reason to break extensions for no strong reason.  My feeling is unless it isn't possible, we should deprecate one release and remove the next.  My guess is that usage of sendMail and sendAdminMail is not uncommon in extensions.

I would support some sort of tool to generate a log of deprecated methods used that devs could use to figure out what changes they have to make to their components.  The other option would be to add it to a legacy layer.

Ian

[Ed Stafford]

I'm with Ian, so far as this goes.

* Move functions into their appropriate areas
* Create a JUtility class to keep backwards compat with 1.5
* Make API notes that the functions have moved into classes XYZ
* Remove JUtility completely in 1.7+

Several folks have mentioned it, and much as I'd like to take a hammer
and chisel to a ton of the code for some reorg, I'm definitely with
keeping backwards compatibility as much as possible for 3PDs.

--
Ed Stafford

[Paladin]

On Nov 17, 8:04 am, Hannes Papenberg <hackwa...@googlemail.com> wrote:
> Just a short note from me: If we move all these functions to another
> class, don't keep an empty class with proxies, but remove the class in
> 1.6 directly. I'd vote for this. Anyone very strongly against this?
> Otherwise: Could you create a patch? :-)

Have every intention of creating patches to do this, but it'll take me
a little while to get to it. Right now I'm juggling three Joomla
projects: this one, the coding conventions issue, and unit tests. The
unit testing approach I was using wasn't working for me, so I'm trying
to (re)build something that does, at which point I'll try and co-
ordinate some changes with Ian and Ed so we can all have it work the
way we want it to off the same testing framework. (At the moment I'm
one file away from that, I think.) By then it'll have simmered long
enough for me to be more confident of the changes, at which point I'll
fire off a half-dozen patch files to get there, and it'd also be nice
to have firmed up some usable coding conventions before NY DevCon so
that The Point Can Be Made.

If there's a reason it can't wait a while longer, let me know and I'll
shift priorities.

As for dropping JUtilities immediately, I'm with Ian that it should
deprecated for at least one release, to give the 3pd's time to fix
their code, but we should start changing *all* the calls in core as
soon as the relocation is in the repo.

[Andrew Eddie]

Hannes, the current practice is to proxy where possible, mark as
deprecated and remove from the next release after this one. Except
for this where we just can't provide a backward compatible route (and
there are valid cases for this), I would generally be against just
dropping things like that.

Regards,
Andrew Eddie
http://www.theartofjoomla.com - the art of becoming a Joomla developer

2009/11/18 Hannes Papenberg <hack...@googlemail.com>:

[Paladin]

On Nov 9, 8:45 pm, Andrew Eddie <mambob...@gmail.com> wrote:
> Agree generally.  I think there's possibly some merit in keeping the
> clear token and then having one for usage in forms.  So maybe
> JUtility::getToken moves to JSession::getFormToken which is static.
> Not 100% sure on that one myself other than it should change.

Been thinking a liitle more on this. On the one had, it's simply a
user.session hash so JSession looks right. But since it's intended for
use in forms, should it be in JForm (or JApplication, in case we want
to use it on more than forms)?

Brain fried, time to sleep on it.

[Paladin]

On Nov 9, 8:45 pm, Andrew Eddie <mambob...@gmail.com> wrote:

> I think there's possibly some merit in keeping the
> clear token and then having one for usage in forms.  So maybe
> JUtility::getToken moves to JSession::getFormToken which is static.
> Not 100% sure on that one myself other than it should change.

I'm moving it into JSession as a static, but while I'm sold on
JSession as the right place for it, I'm not sold on the idea that it
needs to remain a static, since JSession is a singleton controlled by
JFactory. But that can be addressed on the next refactoring pass
through JSession; I haven't spent enough time in JSession at the
moment to be comfortable doing that refactoring at this point.

(That's after all, why I picked up the Unit testing. It gives me an
excuse to ask impertinent questions about every class in the system
while at the same time building a mental framework to start hanging
more detailed knowledge on. The system is complex enough and tangled
enough still that getting to that latter stage will take a while.)

[Paladin]

On Nov 9, 8:45 pm, Andrew Eddie <mambob...@gmail.com> wrote:

> 2009/11/10 Paladin <arlen.wal...@gmail.com>:

>
>
>
> > parseAttributes - "for XML style attributes" Belongs in simpleXML,
> > then, right? Or, if actually used in processing many types of
> > documents (as it actually seems to be) then it should be in the
> > inheritance tree for processing those documents. Either way, not here.
>
> Actually it's for parsing a=1&b=2, etc.  Probably something native we
> can use or bolt it onto the JUri class.

Just so we have a better view of this function:

It's used in html.php to parse the attributes of a <jdoc:include />
line in the templates.

It's used in geshi.php for whatever geshi does. (Never have succeeded
in completely wrapping my head around this bit. Looks like it's the
main engine for replacing tags with values from plugins.)

Finally, it's used in setting up pagebreaks and ToC's in the content.

From this, I don't think JUri is the place for it. Nor can I agree
with JArrayHelper entirely. In fact, since it seems to be aimed at
plugin-style syntax (the jdoc tags in template are another example of
that) I'd tend to think it belongs in either JDocument or
JPluginHelper rather than JSimpleXML, which was my first thought.

[Paladin]

On Nov 9, 8:45 pm, Andrew Eddie <mambob...@gmail.com> wrote:

> Agree generally.  I think there's possibly some merit in keeping the
> clear token and then having one for usage in forms.  So maybe
> JUtility::getToken moves to JSession::getFormToken which is static.
> Not 100% sure on that one myself other than it should change.

Does it *have* to be static, since the JSession object has been
instantiated and sits in JFactory?