Suggestion: Move Laravel Integration to Separate Package about tntsearch

[ptondereau]

+1 for separation of concerns with this repo ! You should leave your library framework agnostic as possible and propose different framework wrapper. Because of easier dependencies project management and unit tests. Laravel is easy to set up but a symfony bundle requires a lot more stuff!

[nticaric]

I agree with you guys so this package will stay framework agnostic. For Laravel 5.3 we created a scout driver

https://github.com/teamtnt/laravel-scout-tntsearch-driver

[dragonfire1119]

What's the difference if the Service Provider is in another Repo or in the current one? I like it in the current because I don't have to require a whole different package.

[stokic]

I see no reason why there should be another package which would act like a wrapper, the package is framework agnostic meaning you can use it in any project, and it's very common for packages to add easier integration for multiple frameworks to make life easier for developers.

What would be the exact benefit of splitting in this exact case? Two files less in your vendor folder? ;)

[jakejohns]

Fair enough. I certainly have been guilty of doing this as well.

My only technical concern is the version issue. This package doesn't require (in composer.json) illuminate/support (and it shouldn't) so there's no version constraint. But in reality parts of the code base do rely on illuminate/support, and also technically on a specific version of that package. Future or older versions of Illuminate\Support\ServiceProvider are not necessarily compatible and that class may or may not even exist in whatever given version of Laravel.

I'm assuming what is assumed here is Laravel ~5.0, or some such faux-dependency. But given this paradigm theres no real way to enforce that.

Additionally, say the Laravel ServiceProvider does fundamentally change it's API, and so the ServiceProvider class here needs to change, but none of the actual core code base relevant to this domain does. Does that mean a 1.0 release for this package? That seems troublesome as well.

Then there is the question of "multiple frameworks", where does the madness stop?! Personally, I admit some of my criticism comes from an aversion to Laravel, but that aside. Lets say you decide to include in this package integrations for frameworks, X, Y, and Z as well. Is there a 1.0 Release every time one of those frameworks updates their DI or configuration API? Does one then supply support patches for new features to each of the old ones as well?

I would argue that what is easier for developers in the long run IS discreet integration packages.
Those integration packages can have their release cycles dependent upon the framework API (as that's what this dependency you're introducing actually is). The integration package is like an interface doing inversion of control for the package manager.

But I digress... :)

[nticaric]

To be honest, you have a very good point here. Let me think about it for a bit. You're suggesting to remove the files in version 0.7 and move them to a separate package?

[jakejohns]

I think this package should look more like this:

tntsearch/src
├── Indexer
│   └── TNTIndexer.php
├── Stemmer
│   ├── CroatianStemmer.php
│   ├── GermanStemmer.php
│   ├── NoStemmer.php
│   ├── PorterStemmer.php
│   └── Stemmer.php
├── Support
│   ├── Collection.php
│   ├── Expression.php
│   └── Highlighter.php
└── TNTSearch.php

3 directories, 10 files

And another new repo, like tntsearch-laravel, should look more like this:

tntsearch-laravel/src/
├── Facades
│   └── TNTSearch.php
└── TNTSearchServiceProvider.php

1 directory, 2 files

With a composer file that looks more like this:

{
    "name": "teamtnt/tntsearch-laravel",
    "type": "library",
    "description": "teamtnt/tntsearch integration for Laravel",
    "keywords": [
        "teamtnt",
        "tntsearch",
        "larvel"
    ],
    "homepage": "https://github.com/teamtnt/tntsearch-laravel",
    "license": "MIT",
    "authors": [
        {
            "name": "Nenad Tičarić",
            "email": "[email protected]",
            "homepage": "http://www.tntstudio.us",
            "role": "Developer"
        }
    ],
    "require": {
        "php" : "~5.5|~7.0",
        "teamtnt/tntsearch" : "~0.6.0",
        "illuminate/support" : "~5.0"
    },
    "require-dev": {
        "phpunit/phpunit" : "5.*"
    },
    "autoload": {
        "psr-4": {
            "TeamTNT\\TNTSearchLaravel\\": "src"
        }
    }
}

If you want the laravel integration, you composer require
teamtnt/tntsearch-laravel, and add

// Service Provider
TeamTNT\TNTSearchLaravel\TNTSearchServiceProvider::class
// Facade
'TNTSearch' => TeamTNT\TNTSearchLaravel\Facades\TNTSearch::class,"

I do think this would be best practice.
Those classes (the service provider and the facade) DO actually require and
depend on illuminate/support. Seems best the package they are in actually
require them

That's my two cents.

[jakejohns]

As noted above, I believe the move to illumunate/support 5.4 breaks this package because it is in fact only formally agnostic, not actually agnostic.

ie. this package, in its current state (the inclusion of the facade and service provider), does currently depend on a specific version of illuminate/support even if composer.json does not acknowledge that fact.

I thought the above note from @nticaric was in agreement with the idea of moving the facade and service provider, but perhaps I am mistaken.

[nticaric]

@jakejohns yes, you are right, the service provider and facade will be removed in the next commit. They were only left there not to break code for those who used it that way. It was even removed from the readme to not encourage users to use the facade. Since the laravel scout driver is out for a while now, I guess it's safe to remove the service provider and facade from code. If there are still some who used the facade, should switch to scout instead.

[jakejohns]

Cool! Thanks for the update/clarification, @nticaric !