NYHTC / FMLaravel

Laravel friendly wrapper for the FileMaker API for PHP
MIT License
13 stars 20 forks source link

Silent FileMaker Errors and Empty Result sets #6

Open tschiemer opened 8 years ago

tschiemer commented 8 years ago

Hi Andrew

I've been working on a fork for a bit now and I would just like to note that I think it's suboptimal that FileMaker Errors (also in your testing branch) get silently discarded.

If there's an error I as a developer/system supporter would like to have a way of being informed about it. Also this makes it easier to diagnose initial installation problems (at first my connection failed due to some authentication error silently but I didnt get much of a helpful indication of that).

I would suggest introducing an exception to inform about FileMaker_Errors.

Also, when performing a find and no matching record is found, FileMaker returns an error (which I find somewhat silly behaviour, although might make sense in the original FM context). So if and when introducing exceptions I would also suggest excluding the "no matching record" FM error (has error code '401'). Possibly there are more similiar cases, but that's the obvious one I encountered.

Edit: Also in the context of verbosity, I would consider connecting the FM API logging mechanism by trying to feed it a PEAR_Logger conforming proxy to the laravel system / psr logger..

andrewmile commented 8 years ago

Hi, thank you for your input and I like the additions you have made on your fork. Keep in mind that FMLaravel is still pre 1.0 so it is not yet finished and I'm also aware of the lack of error trapping. I was doing a lot of work with the FM PHP API several years ago and often wished I could use it as a driver in a modern framework but there just wasn't anything out there. Eventually my company gave me the chance to create FMLaravel and open source it but our needs changed and I have done very little on it since then (although we are using it in production for a few projects). I presented this at DevCon last year in hopes that people would contribute so I am happy some interest is finally being generated. I have not abandoned this project and do plan on picking back up on it but any work I do on it is on my own time which has been limited lately. Thank you again for your input on the project.

tschiemer commented 8 years ago

Hi Andrew

Happy to contribute :) I'm in a similiar position really. For my status, I'd also have the logging working and am looking for comfortable ways to push container field data right now..

Well, I do have one question regarding the script functionality you prepared in Support/Script.php: I'm not sure I see how you ment to use it and in particular where.. would you mind if I change to support static calls such as Script::execute(layout, script, params) or is it needed somewhere I don't see?

Edit Another question: in Connection::getConnection($type) I see you have prepared some different methods for getting specific configurations (read/write/script/auth). I can see that auth is a special case, but I am wondering why the others are needed? I can see the purpose of having different sets of configurations, but do they need some special kind of processing with respect to the FileMaker API or could it be generalized?

andrewmile commented 8 years ago

Hi Filou,

Thank you for the kind words :) I appreciate you putting effort into this project as well and knowing other people are using it motivates me want to do more with it.

As for script.php I prefer to keep execute as a non static method so it can be injected into other classes. This will allow you to mock the script class when you are testing. You can type hint FMLaravel\Support\Script into a controller method or constructor and Laravel will inject it. Then you can call $script->execute(layout, script, params)

Yes, the different connection types can be a bit confusing. The background behind them is the site I originally built FMLaravel for has users authenticate with their credentials, read data with a universal login, and edit data via calling scripts (This is also why writes were never implemented). I want to be able to support different sets of credentials for connections but I'm not satisfied with the current implementation.

As you can see from looking at the code base much of what is there was in order to "get it to work" and there are a lot of improvements to be made. A lot of the difficulty was in the Model and QueryBuilder classes to make them use FM API while extending the Eloquent Model and returning Eloquent Collections / Models. I want to push to get this to a 1.0 release but there is a fair bit of work to do to get it to the point where it covers the full FM API and is testable and stable. As always though thank you for your input on this and in particular the work you are doing on the container fields is a big help!