It would be great if we could add a configuration setting that would allow admins to specify an alternative location for the phsCAS directory. It is problematic to have it in Drupal's root directory because it makes it a little more difficult to upgrade core in some instances. In addition, it is starting to become common practice for modules to add their libraries to the sites/all/libraries directory. Having a setting would allow us to install CAS to the libraries directory, or even specify a path outside of the document root.

Thanks,
Chris

CommentFileSizeAuthor
#14 cas-85794-10.patch0 bytesmetzlerd
#12 cas-857954-9.patch5.29 KBmetzlerd
#8 cas-857954-8.patch5.13 KBcpliakas
#5 cas-857954-6--2.patch (6.x-2) 2.18 KBrealityloop
#4 cas-857954.patch (6.x-3)1.9 KBrealityloop
#4 cas-857954-6--2.patch (6.x-2) 1.89 KBrealityloop
#3 cas-857954.patch (6.x-3)1.9 KBrealityloop
#3 cas-857954-6--2.patch (6.x-2)1.89 KBrealityloop
#2 cas-857954-2.patch5.6 KBcpliakas
#1 cas-857954-1.patch5.38 KBcpliakas

Comments

cpliakas’s picture

cpliakas commented
Status: Active » Needs review
StatusFileSize
new cas-857954-1.patch5.38 KB

The attached patch is a first attempt at the configuration setting. The variable defaults to "sites/all/libraries/CAS", however there is an update function that sets the variable to the old location for people who are upgrading from pre 6.x-3.x versions of the module.

cpliakas’s picture

cpliakas commented
StatusFileSize
new cas-857954-2.patch5.6 KB

I made a mistake and referenced the "phpCAS.php" file in various comments and help text, which obviously doesn't exist. The attached patch fixes the mis-referenced file by replacing with "CAS.php". In addition, it adds a validation handler to make sure trailing slashes are stripped if they were added by the user. This makes it consistent with the path that is calculated in the cas_library_version_get() function.

Thanks,
Chris

realityloop’s picture

realityloop commented
Title: Add setting to configure the location of the phpCAS directory » Add libraries API support
StatusFileSize
new cas-857954-6--2.patch (6.x-2)1.89 KB
new cas-857954.patch (6.x-3)1.9 KB

Your not making use of the Libaries API module which allows this work with libraries directory in multiple places, here is my version of the patch for 6.x-3 and 6.x-2.

Tested and working at my end.

realityloop’s picture

realityloop commented
StatusFileSize
new cas-857954-6--2.patch (6.x-2) 1.89 KB
new cas-857954.patch (6.x-3)1.9 KB

Fix to meet coding guidelines

realityloop’s picture

realityloop commented
StatusFileSize
new cas-857954-6--2.patch (6.x-2) 2.18 KB

6.x-2 patch didn't work, have updated and now it does.

cpliakas’s picture

cpliakas commented

Hi realityloop.

Thanks for the input. From my perspective, the Libraries API is in an alpha state, and I would rather not make this module have a hard dependency on a third party module. I think integration with the Libraries API is a wonderful idea, but I am of the opinion that it should be optional and not required. Just my $0.02, and I would be interested to hear the maintainer's perspective.

Regardless of my opinions, great idea and good work with the patches.
Chris

metzlerd’s picture

metzlerd commented

I would agree. We need a soft dependency here. Since the module doesn't currently support putting stuff in sites/all/libraries, I'm ok with adding an if function_exists test to determine whether that library is installed, but the require_once should work a in a way that it doesn't break existing installs that have the CAS library installed just by adding it to the PHP include path. I don't really want to force people to install stuff in the library folders here.

Does that make sense?

cpliakas’s picture

cpliakas commented
StatusFileSize
new cas-857954-8.patch5.13 KB

Hi metzlerd.

To me, the reasoning makes sense. I think the goal of the patch in #2 is to eliminate any assumptions all together since it gives the developers the option of where the CAS library resides. I re-rolled a patch that is a combination of mine in #2 and realityloop's in #4 to add Libraries API support. In addition, I modified the logic so that it works exactly as it does now by default. Judging by the response in #7, this is important. With the patch applied, users will be able to do the following:

  • Include the CAS library by modifying the include path in php.ini (as is recommended in the current setup).
  • Put the CAS module in the libraries directory similar to the WYSIWYG module's approach.
  • Allow the developer to put the library anywhere, including outside of the Drupal root directory, without modifying the include path. This eliminates the need for any custom code if the user does not have access to php.ini.
  • Integrate with the Libraries API and allow that module to manage the CAS library.

Thanks,
Chris

cpliakas’s picture

cpliakas commented
Title: Add libraries API support » Add setting to configure the location of the phpCAS directory

Changing back to original title for consistency with what is trying to be accomplished in this issue.

realityloop’s picture

realityloop commented

Tested and working using libraries method

metzlerd’s picture

metzlerd commented
Status: Needs review » Needs work

Did not work as it currently does by default. I'm going to try a bit of a logic rework and submit a revised patch for testing by others.

metzlerd’s picture

metzlerd commented
Status: Needs work » Needs review
StatusFileSize
new cas-857954-9.patch5.29 KB

Here's a revised patch. I changed the code to use class_exists to detect failure. Would love to see some more testing on this in other enviornments.

cpliakas’s picture

cpliakas commented

The addition looks fine to me. I think defensive coding like this always helps the stability of a module.

Thanks,
Chris

metzlerd’s picture

metzlerd commented
Status: Needs review » Fixed
StatusFileSize
new cas-85794-10.patch0 bytes

Further testing revealed that I couldn't save any settings without revising the test to ensure that the file exists. So I made a pretty significant revision to to the strategy, here. I'll go ahead and commit these changes, since they appear to work. But additional testing would be welcome.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.