Bug #984
closedCheck for config.php in leginon.inc uses hardcoded Base URL
0%
Description
This broke the Goby installation since it was not installed in myamiweb.
We need a different solution here. Related to #535.
Updated by Eric Hou about 14 years ago
- Status changed from New to Assigned
- Assignee set to Amber Herold
Can you address the problem more detail? I don't understand what it means by Goby was not installed in myamiweb? (what is Goby installed?) and where is the hardcoded URL? (are you saying line 24 in leginon.inc ?) If that is what you refer to, then there is no easy solution for it. Because the searching is button up until it reaches to myamiweb folder. If I don't put a stopping point, then it will keep going up to all the files in the web server. And the stopping point is the myamiweb folder because it is the top of the project folder in the web server.
myamiweb is the required name to use at this moment.
Thanks.
Eric
Updated by Amber Herold about 14 years ago
- Status changed from Assigned to In Code Review
- Assignee changed from Amber Herold to Eric Hou
- Target version changed from Appion/Leginon 2.1.1 to Appion/Leginon 2.2.0
Eric, I moved the redirect to myamiweb/index.php and changed the require_once "config.php" to includes so I could get a return value and print out a message if the config is missing. This tested out fine, but I was wondering if there was a problem with doing the redirect in index.php, I remember you had a problem with redirects in the past. Also, I know we try to stick to using require_once, but I think it will be OK in this case. The config file could get included multiple times but it doesn't look like that will be a problem. These changes will allow the web parts to be installed in a folder other than myamiweb.
If we do not stick with this code, the other option is to enforce installation into myamiweb by giving an error in the setup wizard when it discovers the base path does not end with myamiweb.
Updated by Eric Hou about 14 years ago
Just from reading the code you changed, are you just printing out an blank page with error message with explain where and how to create the configuration file if the config.php is not exist?
Why do you want to change to something for user to do things manually if I already did programming automatic why to direct user to the right place? (Don't go backward please!!)
Second, we do have many more places are checking if the config.php exist or not, so your changed might not be enough to cover the entire system. And if all the place need to check config file need to add those echo messages. It would become a serious problem to change all the error messages everywhere. We probably have more 50 file require or include config.php.
The other thing, the reason you do not get error message from require function is because require() throw running time error and we turn that off in the php.ini in the production server which should not throw any programming error message to the customer. But we did not turn off the warning therefore included() will print out the error message for you.
require function throw run time error message and stop the program. require function load all the files at the beginning of the program.
include function throw run time warning message but program will keep running. include function load files during the run time when reach to the line.
require_once and include_once means to include or require file just once.
I do have an better way to solve this problem, but I can't do it while I am not in the office.
Something about this kind of globe change require lots of testing. Example you need to test all php pages from all the different folders and sub-folders in the file system on the web browser.
You need to look at the file system
The other thing about using exception in php.
Exception is for debugging purpose in web application development. Normally we only use in development, or testing environment. In production server, it need to turn off all the Exception error.
Web application require an presentation tier to display proper format to the web browser. So if you use Exception to display error message to the user, there would be no format and sometimes it will also break the web template (for example php generate the first part of the web template (template header), and during the business logic, there is an exception occur, then the footer template did not get load.
Good place to use Exception is to check all the I/O connection exist or not. Which we should stop the program if there are I/O errors and we should put it in the log file.
Don't throw Exception back to the user, we should only throw Exception to the log files. Because it is very hard to manage in the feature (for example, we need to clean all the Exception message every time we need to release the software) unless you want to make sure all the Exception error message in proper HTML format. This is very very hard, because you will need to deal not only HTML, you also need to deal with CSS and javascript. And this also lead us to very tight coupling programming. (very very bad).
Let's try to write more quality code, and loose coupling code. So we can do unit testing in the feature.
Hope above will help.
Eric
Updated by Eric Hou about 14 years ago
- Status changed from In Code Review to In Test
- Assignee changed from Eric Hou to Amber Herold
Updated by Amber Herold about 14 years ago
What is the status on this? Did you want me to change the code or assign this for testing?
Updated by Eric Hou about 14 years ago
Not so sure. I guess just keep the way is it for now.
Eric
Updated by Anchi Cheng about 14 years ago
I am having problem with this new method at NIS test site. This base_url is included in the config.php. It seems that the new code made config.php read too late so that processing/index.php didn't go through.
Will talk to you and Eric after I understand the problem better.
Updated by Anchi Cheng about 14 years ago
Changing include to include_once made it work at NIS with some relocation of files.
Updated by Amber Herold about 14 years ago
- Status changed from In Test to Closed
Tested.
Not merging to 2.1. Does not seem high priority enough.