Better handling for determining cas library path

I disagree that the file test is the correct strategy. The current patch works when the file exists in the explicit directory but fails when the library is installed via an include path in php.ini such as a Pear install. The class_exists call covers the case such that the code fails gracefully if the cas library didn't find the correct file. We use this technique to make sure that we don't have to keep reinstalling the phpCAS library for every php product that we use to install it. If we need to suppress warnings, then we should do that and throw a graceful error message.

Hey Adam:

I agree this is one of the sorer parts of the CAS module (any for that matter, any module which uses other libraries). Nonetheless, we cannot add the file_exists check as it is possible for CAS.php to be in the include_path without being in an open_basedir allowed directory... go figure.

One area that could see improvement is the Libraries API / manual path entry selection. Currently the methodology is:
- If Libraries API is installed, use that to locate CAS.php
- If not, present a text field for the library location.

It'd probably be better to structure this as a radio field:

( ) Use Libraries API to locate CAS.php.
(x) Manually enter CAS.php path:
    Relative or absolute location of CAS.php: [                                        ]

We could hide the text input field if that option isn't selected. Additionally, we could disable the Libraries API choice if that module isn't installed.

Hopefully this is something that Libraries API 2.0 will help us manage more effectively, but it hasn't been released yet.

What sort of underlying issue were you having that lead to this?

Okay, so this is a quick mockup of my suggestion in #3. Can you play with this a bit and provide some feedback? I think it should work for your use case -- just set the variable cas_library_use_libraries to 0.

Adam, thanks for the review. I can certainly remove the periods in the next iteration.

Dave, care to chime in here?

I didn't test the "upgrade path" --- but I think it should be fine --- there is no hook_update_N, and the new variable defaults to the old behavior.

I'm sure everything looks terrible with Javascript turned off... is this a concern? I think we should change the wording of the titles of the form elements --- the radio button title might be okay, but the titles below shouldn't both be "phpCAS library path". Suggestions?

Lastly, I suppose this should be backported to 6.x-3.x, which doesn't have the '#states' form support (i.e., the auto hiding / showing of the various sub options). Maybe we should design the text for that consideration, which would also then satisfy the "no js" crowd...

Since this is "admin" oriented. I don't have a problem with the look with Javascript turned off. Site admins with Javascript off should be a pretty small percentage indeed, but given the need for a 6.3.x backport you may be right about redoing the language.

I'm good with the approach. I did the upgrade path testing and it works as advertised.

Since the fieldset is already titled PHPCas Library, perhaps the fields should simply be titled something like , "Detection"
and "Path"?

Setting to needs work since we need to both:
- Fix the language
- Backport to 6.x-3.x

I have yet another approach using the Drupal cache.

The first thing I do is checking if phpCAS has been already loaded, then we don't need to do anything. Currently that check is being done last. Some said that the library may have been loaded by other non-Drupal means, i.e. PEAR, but actually the current implementation will fail in that case, because the file include alway happens. My implementation takes care of that case too.

If a path is provided, that path is being used. If no path is provided, I look in the Drupal cache for the library filename. If it's there fine, if not use the old way to to find CAS.php, but additionally look in subfolders of the library path. That takes care of the case where drush make is used and phpCAS is automatically extracted with an additional subfolder, e.g. 'CAS-1.3.2'.

If we still can't find CAS.php, just accept the fact and provide an error message. If it has been found, cache the resulting filename, so we don't have to look for it again.

This should take care of every case except when you use the library module, but don't put phpCAS in any library folder and don't use php.ini or similar to load it. But I think this should be discouraged anyway.

+    if (!file_exists($filename)) {
+      // library not found at path, drush make extracts it into an additional subfolder, so also look there
+      $files = glob($path . '*/CAS.php', GLOB_NOSORT);
+      
+      if ($files !== FALSE && !empty($files)) {
+        // CAS.php has been found at least once, use first match
+        $filename = $files[0];

For performance reasons this bit of code is a non-starter. Glob is relatively expensive and we shouldn't risk doing it on potentially every page load. The user will need to properly configure the variable instead.

I think in this case glob will be fine, but feel free to use a different method to look for CAS.php in a subfolder, usually we will be iterating over one file and one directory, package.xml and CAS-n.n.n, so I don't think the method will matter. But I still agree that we must avoid doing that on every page load, thats why I used Drupal's cache. So once we found the library, we don't have to do it again until the cache is cleared. That is actually an improvement to the current solution.

I think configuring the variable is not an option when using the Libraries module and drush make to download the library.

I created a drush make file that I use for my personal distribution, but I thought it might better be served here. I had to use the Github URL because drush make will not let you pass tar CLI attributes.

Marked #2053233: Allow custom path, even with libraries module enabled. as a duplicate. The patch in #5works perfectly, will reroll shortly to reflect feedback from #9.

Re-rolled #5, with change in language.

OK tested this myself and looks good and seems like a much better way to handle this.

Based on #9, this needs a backport to 6.x-3.x.

With the patch in #5 applied, the following steps result in a WSOD:

1. install phpCAS at sites/all/modules/cas/CAS
2. install libraries api and enable it
3. visit /admin/config/people/cas and notice that the form defaults of (Provide an absolute or relative path and phpCAS library path=CAS) result in "phpCAS version 1.3.2 successfully loaded."
4. do this:
cd sites/all
mkdir libraries
mv modules/cas/CAS libraries
5. at /admin/config/people/cas change selection to Locate Using Libraries API. Submit the form and notice that phpCAS is detected.
6. do this:
# pwd = sites/all
mv libraries/CAS modules/cas
7. reload /admin/config/people/cas and get a WSOD
8. recover from the WSOD with:
# pwd = sites/all
cp -r modules/cas/CAS libraries

bwood, do you use APC (or another opcode cache)? I ran through your steps, and with a slight modification, got the WSOD you reported. I couldn't get any error messages out of php or drupal, but I first noticed that restarting apache (which I did to change some error reporting in my php.ini) caused the WSOD to disappear. That made me think it was APC, and sure enough, if I follow your steps, get a WSOD, then clear my opcode cache, it works again. I'm pretty sure that opcode caches aren't huge fans of classes being moved around.

Currently, you can get the WSOD behavior with APC by:

Installing CAS without Libraries module.
Putting the phpCAS library at modules/cas/CAS
(Check the CAS settings form; it will be found and loaded)
Install Libraries module
(Check the CAS settings form; it will give error message)
Move phpCAS to sites/all/libraries
Any admin/config path will now give WSOD until the opcode cache is cleared

Hi Tim,

Yes was using APC.

My thoughts about a fix:

In cas.admin.inc: cas_admin_settings do a file_exists on vget('cas_library_dir') if false do something like set cas_library_dir to NULL and print a warning. I haven't dug into the code. That's just off the top of my head. Thoughts?

Not sure this is worth coding around. Admins with op code caches should understand this. Copying libraries instead of moving them does avoid the problem.... maybe just having this page to document the issue is enough?

I hesitate to add complexity to the loader just to solve this edge case?

I'm not quite sure it is possible to code around in the manner described anyway. I agree that we should let it be; moving code around arbitrarily will cause problems, and should be expected to do so. We can't concern ourselves with every possibility anyway.

Patch #5 and #15 seem reasonable to me, and think the WSOD is not related directly.

I have created my own patch for this that I will post here as a simplified version. For me the nice UI in the admin page is not of value, I would rather keep all variables the same. Which my patch allows for, it will just roll back to the variable path if CAS is not able to be loaded via libraries.

For me the real issue here is that you can't "not use libraries", so a perfect and functioning CAS site will be broken if you enable the library module without adjusting the CAS settings.