Closed (fixed)
Project:
Pathauto
Version:
6.x-2.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Reporter:
Created:
11 Nov 2010 at 12:58 UTC
Updated:
31 Dec 2010 at 22:00 UTC
Jump to comment: Most recent file
Please restore the _pathauto_include function before making a 2.0 release. It seems like it is or at least used to be part of the official API and there's still mention that people should be using this function in the pathauto.inc file header. It got removed in pathauto.module in revision 1.158, but unfortunately the commit message doesn't mention the removal of this function.
The latest release of Ubercart (6.x-2.4) depends on this function being available in the uc_catalog submodule. It gave me a white screen of death and I saw this error message in the logs:
Call to undefined function _pathauto_include() in path/to/modules/ubercart/uc_catalog/uc_catalog.module on line 525
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 968794-pathauto-restore-deprecated-func.patch | 1.1 KB | dave reid |
| #13 | pathauto_include_deprecated_watchdog.png | 35.98 KB | dave reid |
| #10 | 968794-pathauto-restore-deprecated-func.patch | 780 bytes | dave reid |
Comments
Comment #1
dave reidIt wasn't ever an API function. Modules that are calling this function will still not work with the latest code, so they need to be fixed.
Comment #2
dave reidJust committed to both 6.x-2.x and 7.x-1.x to fix the reference to _pathauto_include() to use the proper method of module_load_include('inc', 'pathauto');
Comment #3
gregglesI'd rather go the other way. That function clearly seems useful to me if others are using it so we should remove the underscore from the beginning to make it possible for others to use it.
And where's the patch you committed in #2? And where's the link to the cvs commit entry? If we look at most of the page doesn't have issues. I'm concerned about the best practices.
Comment #4
dave reidGah, sorry about that Greg. It was a documentation only change, so I didn't post a patch.
http://drupal.org/cvs?commit=449426
http://drupal.org/cvs?commit=449428
I'm worried that by adding this function back in the 6.x-2.x branch that we're continuing to hide the problem and not encouraging modules to fix their code as nothing would be evident that calling the function is doing the wrong thing.
Comment #5
gregglesWhen you say "hide the problem" what is the problem we're hiding?
Comment #6
dave reidThat modules should be using module_load_include('inc', 'pathauto') rather than _pathauto_include() to get access to the pathauto_create_alias() and other functions that they want to use.
Comment #7
Peter Bex commentedSorry for the late reply, it just occurred to me that I should have a look at the issue list.
module_load_include does seem the "proper" way to do it. Having one huge function which loads everything doesn't strike me as particularly elegant. It's better if people only load the files that are absolutely necessary.
Originally I requested for restoring this function, but that's just because people are already using it. It could be marked "deprecated", and maybe even log a warning. At least keeping it around for a few months so that the ubercart people can get their act together would be a good idea. Otherwise people risk major breakage just updating pathauto.
Comment #8
gregglesI agree with Peter's proposal.
This breaks too much to just rip out, IMO: http://drupal.org/search/apachesolr_multisitesearch/%22undefined%20funct...
Comment #9
dave reidI'm coming around on this, so I think it will be a good idea to put it back in.
Comment #10
dave reidComment #11
gregglesI think this will still cause fatal errors because people will assume code is included that isn't.
I basically feel we should revert the previously committed code and just add a watchdog saying it's been deprecated.
Comment #12
dave reidTrue, we only need to add module_load_include('inc', 'pathauto') since the token functions work by default and do not need includes.
Comment #13
dave reidRevised patch that adds a watchdog notice, with helpful information about what file/function/line called _pathauto_include() so it can be reported back to the offending module. I added a test call to _pathauto_include in the token list function and attached screenshot is the watchdog message.
Comment #14
dave reidComment #15
dave reidTalked with greggles on IRC and he liked it, so I'm committing #12 to CVS.
http://drupal.org/cvs?commit=466184