FriendsOfCake / search

CakePHP: Easy model searching
MIT License
170 stars 59 forks source link

SearchComponent conflict with DisableGenericTables #312

Closed kevinquinnyo closed 2 years ago

kevinquinnyo commented 3 years ago

In the SearchComponent, here https://github.com/FriendsOfCake/search/blob/master/src/Controller/Component/SearchComponent.php#L103, there is an attempt to resolve the underlying model from the controller that is using the search component, and set some custom view variables.

There is a note that says:

      * You need to configure the modelClass config if you are not using the controller's                      
      * default modelClass property.   

We tend not to use the ModelAwareTrait very often, and have several controllers in our application that aren't named in a way that could be inflected through convention to the underlying table, so this is a problem for a large number of controllers for us.

Right now the problem for me is that I'm upgrading an application from cake 3 to cake 4 and we (long ago) added the DisableGenericTables patch advised in the cakephp 3 docs here: https://book.cakephp.org/3/en/development/configuration.html#disabling-generic-tables

As far as I can tell this remains a valid solution if you don't want generic table and entity classes and wish to avoid typos like 'Atricles' when manually using the TableRegistry directly for instance. Perhaps I'm wrong about this though?

I believe what's happening here is that in the SearchComponent, it only catches UnexpectedValueException so this InternalErrorException is bubbling up.

I can definitely workaround this in a number of ways (manually adding $modelClass = '' to all of our controllers that don't use ModelAwareTrait would be one way, or adding that to the config for the component when we load it), but I thought I'd ask if you think it's worth catching a more general exception there, or possibly another solution for others like me to make the cake 3 -> 4 upgrade process a little smoother.

Thanks for your work on this plugin!

edit: workaround for the moment:

We have a base controller that extends \Cake\Controller\Controller:

// call this method in beforeFilter() (after SearchComponent likely initialized, but before the component's beforeRender())

    private function shimSearchComponent(): void
    {
        $components = $this->components();

        if (!$components->has('Search')) {
            return;
        }

        $search = $components->get('Search');

        try {
            $modelClass = $this->loadModel($this->modelClass);
        } catch (InternalErrorException $e) {
            $search->setConfig('modelClass', '');
        }
    }
ADmad commented 3 years ago

Why don't you just disable the beforeRender event for the SearchComponent?

https://github.com/FriendsOfCake/search/blob/67aaee581c712ca344a2c8dab3015237324eaf6e/src/Controller/Component/SearchComponent.php#L35-L37

ADmad commented 3 years ago

Right now the problem for me is that I'm upgrading an application from cake 3 to cake 4 and we (long ago) added the DisableGenericTables patch advised in the cakephp 3 docs here: https://book.cakephp.org/3/en/development/configuration.html#disabling-generic-tables

FYI this feature is in-built in Cake 4: https://github.com/cakephp/app/blob/ea27790a4fb399e7490c308d053ef874a72432c7/src/Application.php#L55.

kevinquinnyo commented 3 years ago

Right now the problem for me is that I'm upgrading an application from cake 3 to cake 4 and we (long ago) added the DisableGenericTables patch advised in the cakephp 3 docs here: https://book.cakephp.org/3/en/development/configuration.html#disabling-generic-tables

FYI this feature is in-built in Cake 4: https://github.com/cakephp/app/blob/ea27790a4fb399e7490c308d053ef874a72432c7/src/Application.php#L55.

Nice! I missed that.

kevinquinnyo commented 3 years ago

Why don't you just disable the beforeRender event for the SearchComponent?

https://github.com/FriendsOfCake/search/blob/67aaee581c712ca344a2c8dab3015237324eaf6e/src/Controller/Component/SearchComponent.php#L35-L37

That's probably a better option for us. Thanks for the tip.