Saving of models and refactoring

[tschiemer]

Hi Andrew

So, this is my proposition for the moment being (have to focus on my actual project now..)

I also made some changes to your code (ex. connection handling) as I thought makes sense. I'll try to list:

• changed namespace and calling of Script
• Script param preprocessor is not FMHash anymore (which I also removed as a project requirement) as I wasn't sure how to use it. But it should be easy to re-inject.
• Connection instantiation changed, in particular w.r.t. to Auth\Auth.php
• added support for container fields
• save (insert/update) operations are now fully supported including a sample container field upload handler/script
• logging redirection to laravel logging system is possible
• PSR2 code style

For an overview you can also have a look at the readme

For the moment being I did not include your refactoring from the testing branch, and also there are no unit tests..

Edit Ah yes, and I did not understand how to use the authentication service provider.. so I don't know if it still works as intended..

[bossroom]

Hi guys, yesterday FM has released the new version 15. Here the API in bundle with the new installer: https://dl.dropboxusercontent.com/u/13279096/Absolute/API/FM_API_for_PHP_Standalone.zip

[tschiemer]

Hi bossroom

Thanks for the link! After quickly browsing through the FileMaker.php source it would seem they didn't change anything much or did they? Don't see a version number or anything...

[andrewmile]

Hi Filou,

Thanks for the PR and all of the work you have put into this. I will need to take some time this weekend to review. I'll have to put some thought into how to handle some of the breaking changes though as I (and others) have production projects that are based on the current version.

In particular, I don't want to completely remove FMHash as project dependency as being able to send a hash as script parameter adds a lot of value to calling FM scripts and there are projects that depend on this. Check out this post for usage of these dictionary hashes: http://sixfriedrice.com/wp/filemaker-dictionary-functions/. I do however agree that the developer should not be locked in to using FMHash. I need to extract a Hasher interface for this class so I can allow the use of FMHash to be optional. Additionally, the developer might want to write (or use) a different Hasher class to use instead of FMHash. Either way, I can't remove it completely because projects depend on it.

I am working to get this project up to a 1.0 release with unit tests and your PR will help but please be patient while I review it.

Thanks again for the work and I'll be in touch :)

[tschiemer]

Hej Andrew!

I'm not in a hurry, so no need to hurry anything because of me..

Thanks for the link about the dictionary hashes. I can see it being used but I didn't see how to use it straight away - the most simple approach for me would have been to use the GetValue(Get(ScriptParameter); XY) approach where each parameter would reside on a separate line (obviously only possible for oneline-parameters)..

I did in fact not consider production systems, but I believe there shouldnt be many breaking changes. Actually, I believe the only breaking changes are related to the Script and Auth classes; most if not all other features are optional and have to be setup manually to cause any effects - and in the current version write operations are not supported (and would actually raise exceptions) so i would assume it to be save to say that this should not cause any breaking in unchanged code (I think?). Mhm, I also changed Connection::getConnection() into Connection::filemaker() because I felt the naming to be a bit unclear - but this would be breaking only if any calls to getConnection come from outside of the FMLaravel package.

As parameter hasher for scripts, you'll see I just introduced a callable field without any interface definition whatsoever. I would think if the default parameter handler is changed to use FMHash this problem can be solved, but I guess I'd also make this configurable somehow (more because I'd rather not have to set a hasher/preprocessor for every instance). Further I can imagine there to be a problem with the Script and Auth constructors, instantiation respectively.

Thinking about it, I believe I'll also tweak the LogFacade a little bit such as to not require any inheritance - didnt think of using namespace aliasing..

Well, have a nice weekend! ;)

[Sam-R]

@tschiemer Thanks for your work and creating a pull request - without it I wouldn't have found your additions. I'm keen to get a test environment up to try out your changes.

Of course thanks to @andrewmile for creating the project in the first place, it's been very useful.

@bossroom Thanks for sharing the FileMaker 15 file, I've not had access to FM15 yet to investigate changes. Doing a diff on the v14 vs v15 yielded no results so they're identical which is reassuring to me (we have several external interfaces running FM Server that I wouldn't want to have to re-write).

[bossroom]

@Sam-R you are welcome. Yes, no changes in FM15 PHP API. Guys I believe in this project Laravel+FileMaker, but I'm very new in Laravel and I cannot help you with this repo. I follow you and I can give you full support with FileMaker environment.

[andrewmile]

@Sam-R Thanks for your kind words and I'm happy you are getting value from this project.

This is a pretty big PR and I am reviewing it to add the new functionality. No ETA yet but hope to have these new features out soon.

[tschiemer]

:) happy to contribute and curious about any feedback

Just been browsing the code a little bit and noticed some naming inconsistencies regarding the optional containerField__s__XYSettings (missing/existing s) on the model properties and getters. Not on my working machine so I'll fix that later..

Ah and just FYI: there are two traits for models that I didn't mention in the readme and that you might stumble accross when looking: Database\ContainerField\RunScriptOnSave and ..\SetMutatorBase64Fields . These are more relics from testing approaches for uploading container field data and are also not documented in the main readme. The idea behind them was to have a set mutator for container fields that saves the filename and data into two separate model fields (that get saved as any normal field) and then when saving a model a FM script would be automatically called with a list of the updated container fields - similiar to the RunBase64UploaderScriptOnSave trait as documented. I thought I'd leave those (unfinished and untested) traits in the repository for any developers that wanted to have a go at those - in particular because I read about this being a common or occasionally suggested setup for uploading files into container fields. So, any input/changes on these is naturally welcome ;)

[marcusadolfsen]

Hi!

Really nice work Andrew and awesome additions Filou!

I'm following this with great interest. I have a rather big project which involves FileMaker to be a CMS for our customers end users. Every end user have their own site and can setup complex forms and page build-up. I started using FMLaravel to fetch data and it worked great but since the solution requires data from the forms to be saved I used L5SimpleFM (https://github.com/chris-schmitz/L5SimpleFM) to add, edit and delete. And also for more complex searches. Now that the project is about 60-70% complete I am having huge performance issues with certain requests using L5SimpleFM. When I compare a search with FMLaravel and L5SimpleFM we are talking 3-5 seconds in difference. 5 seconds to load a page is not ok. Also I like the FMLaravel syntax a lot more and want to use it solely and drop L5SimpleFM.

I have experience with the FileMaker PHP API but I'm still new to Laravel so unfortunately I don't think I will be able to help you with this repo. But I'm looking forward to any changes and I will test them right away.

Best Marcus

[andrewmile]

Hi Filou,

I have reviewed your pull request and while I appreciate the work you have done I am unable to merge the request at this time. As I mentioned before this is a large PR and since there are no automated tests backing this project it means I need to test every change by hand which makes a PR of this size very difficult to merge. You have added some much needed functionality to the library but you have also taken a number of liberties with refactors and changing of existing features that I'm not prepared to merge in, especially without automated tests.

I have been working on a rewrite of this project with the primary goal of cleaning up the code so it can be testable. Once it is at that point I will begin adding new features to fill out the missing support for the FM API.

If you prefer to continue maintaining and using your fork that is fine, or if you want to wait until my newer release is finished and then see where your fork differs we could look at merging in new features at that time. If you would like to contribute further I welcome the help but I need the project to be testable first and then we can come up with a plan to release functionality in smaller chunks.

[tschiemer]

Hi Andrew!

Okeli dokeli, I see what you're saying.

Well, my reason for starting coding in the first place was that I needed some features that weren't there for a current project that doesn't allow me to wait. I don't know wether I will have time in the future for contributions, but I guess the code is there and won't be lost so quickly whatever will happen with it ;)

[tschiemer]

Oh, and just an idea for the future.. (posting it here, as I'm not sure where to otherwise)

The FMAPI config file suggests that it can use an arbitrary record class to represent FM records, ie

/**
 * The PHP class to use for representing Records
 */
$__FM_CONFIG['recordClass'] = 'FileMaker_Record';

I think it might be meaningful to create a record class for fmlaravel to avoid any unnecessary loops - like iterating over all fields to copy the data field by field into the result set. ( I mean, I think the data is stored as associative array anyway (but the native class only has a getter for single fields), so why not return the array directly as is?)

[andrewmile]

That's an interesting idea although it would involve editing that config file directly, which makes me nervous. I am in favor of getting rid of a loop but ideally the FMAPI would remain untouched so future versions can be dropped in.

As far as returning the array of fields, the nice thing about having the objects as Eloquent models is it gives you the nicer active record syntax and you can always call toArray() on the model if you want an array.

[tschiemer]

I actually think the config file does not have to be edited :) The FileMakerImplementation does seem to automatically load the config file, yes (https://github.com/andrewmile/FMAPI/blob/8fa19f97cf2752eff4af427e92b7d34ed617cf0f/src/FMAPI/FileMaker/conf/filemaker-api.php, line 20) but what the implementation does afterwards, is just call setProperty(key, value) with the given settings, that also store the default host, username, password, loglevel settings and which can be overriden by manually calling setProperty.

And I think that's a misunderstanding here: I'm totally for Eloquent models and would never want to have them removed. What I ment was to introduce a little simplification when processing the FM records to prepare them for the Eloquent models. Ie in https://github.com/andrewmile/FMLaravel/blob/master/src/FMLaravel/Database/QueryBuilder.php line 40ish in get(..) make it possible to replace

$row = new stdClass();
foreach($result->getFields() as $field) {
    if($field) {
        $row->$field = $record->getField($field);
    }
}
$rows[] = $row;

with

 $rows[] = (object)$result->getRawFieldsArray();

(or would it actually be possible to return raw arrays without having to cast to an object/stdClass?)

As this is pretty much at the core of each and every query returning records and scales with both result set size and field count I'd try to save some processing power, although that might be neglectable....... hush..