codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.33k stars 1.9k forks source link

Bug: Filter auto discover for subproject doesn't behave as expected #4572

Closed chez14 closed 2 years ago

chez14 commented 3 years ago

Describe the bug Filter.php from subproject is not loaded as expected. That causes the CI throws pnd_auth filter must have a matching alias defined. exception while the Filter is correctly defined in Filters.php subproject.

By exploration, I found that when the exception triggered, the Filter.php is not loaded. But when the routing doesnt have filter, the Filters.php is correctly loaded.

Loaded Filters when exception thrown: image

Loaded Filters when no exception thrown: image

CodeIgniter 4 version Based on composer.lock file on both (the main app and the admin panel) uses v4.1.1.

Affected module(s) I think it is the discoverFilters() function from /system/Filters/Filters.php.

Expected behavior, and steps to reproduce if appropriate Filters.php on sub project is loaded before the routing starts working, so when accessing /admin/panel route will redirects to /admin/auth/login.

Step to reproduce:

Here's the minimal setup: https://github.com/chez14/ci4-routing-filter-bug

Context

MGatner commented 3 years ago

Your repo is a little hard to understand because there are multiple app folders, but I think you are missing a step. Your actual Auth filter can be anywhere you want, as long as you create the alias appropriately. In your extension namespace (e.g. MyAuth) you will have your filter (e.g. src/Filters/Auth.php) and the discovery Config file to apply the alias (e.g. src/Config/Filters.php). All the Filters file will do is supply the alias:

$filters->aliases['auth'] = 'MyAuth\Filters\Auth';

I actually just made such a module for my new Menus module, you can use it as an example: https://github.com/tattersoftware/codeigniter4-menus/tree/develop/src

paulbalandan commented 3 years ago

I am assuming here that main-app is your application's point of entry and extendables is where you get the goodies. I see that you have pointed out the problem but it is not a framework fault but an implementation error. Filters discovery needs to iterate over the known namespaces and then proceed to load the found Filters.

In your case, since the two apps are basically standalones, main-app has no way of knowing the namespace and location of extendables. Even though they are both Composer enabled, since main-app is your point of entry, extendables is isolated and out of reach.

How to approach this? You need to map the location of extendables to your main-app. In your main app's Config/Autoload file, you need to setup the namespace and path of your extendables in the $psr4 array property.

public $psr4 = [
   // other namespaces here
    'Extendables' => '../../../extendables/app', // adjust the path here
];
chez14 commented 3 years ago

Hi @MGatner,

Thanks for the suggestion, unfortunately it still didn't work. I tought Filters.php should always be a class, but apparently not, hehe. Well still have no luck. To check the implementation, please see f0c64f89d67b37b6b56f0373d55f8ec9ec690ecd.


Hi @paulbalandan,

I am assuming here that main-app is your application's point of entry and extendables is where you get the goodies. I see that you have pointed out the problem but it is not a framework fault but an implementation error. Filters discovery needs to iterate over the known namespaces and then proceed to load the found Filters.

Sorry for the lenghty reply, yes that is correct. Ahh ok ok. I've read the Autoloader.php and i have no clue either why it didn't discover the Filters.php untill it able to go to /admin/auth/login. When accessing /admin/panel Filters.php from extendables is not loaded, but when accessing /admin/auth/login the Filters.php is loaded correctly :|

In your case, since the two apps are basically standalones, main-app has no way of knowing the namespace and location of extendables. Even though they are both Composer enabled, since main-app is your point of entry, extendables is isolated and out of reach.

Ah, you see, I used the Path-based Composer Package to setup the dev environment because when the admin panel get published, we can just composer require chez14/ci4-admin-panel right away and get it working with minimal effort.

The configuration can be seen on composer.json on main-app, the package also have been installed on main-app, and also the extendables project's composer autoload have been configured. I didn't rename the app folder of extendables just because i want to write shorter reproduce steps.

How to approach this? You need to map the location of extendables to your main-app. In your main app's Config/Autoload file, you need to setup the namespace and path of your extendables in the $psr4 array property.

I've tried this and the Filters.php file still didn't get loaded correctly.


I also have updated the issue description to add the reproduce step and added more information on the sample project. If you need me to test something on my end, please let me know.

Thank you so much, and again, sorry for lengthy reply.

Thanks!

paulbalandan commented 3 years ago

I have cloned your project. And here are the steps to make it working (but I found a bug maybe in your code, well I don't know).

  1. Rename the namespace of your Auth filter to Extendables\Filters. You are using Extendables\Controllers which is violative of PSR-4 and maybe why that is not being discovered.
  2. In your Config\Filters in main-app, add the alias of your Auth filter like this:

    use CodeIgniter\Filters\Honeypot;
    +use Extendables\Filters\Auth;
    
    class Filters extends BaseConfig
    {
    /**
     * Configures aliases for Filter classes to
     * make reading things nicer and simpler.
     *
     * @var array
     */
    public $aliases = [
        'csrf'     => CSRF::class,
        'toolbar'  => DebugToolbar::class,
        'honeypot' => Honeypot::class,
    +       'pnd_auth' => Auth::class,
    ];

You should now get your filter being discovered but you'll encounter a bug in your code thrown by the error message:

CodeIgniter\HTTP\Exceptions\HTTPException
route cannot be found while reverse-routing.

This may be a bug in redirect(route_to('pnd_login')); in your before method and how you set the route alias in Routes of extendables. How to fix it is, well, out of scope of this issue report.

paulbalandan commented 3 years ago

image

MGatner commented 3 years ago

Stellar support @paulbalandan, thanks for the thorough response. I believe this issue has moved out of the scope of a "bug" but my takeaway is that our User Guide is lacking information on Filter discovery. Let's leave this issue up until that's in place.

chez14 commented 3 years ago

Hi @paulbalandan,

Thanks for the thorough suggestion and the fix.

  1. In your Config\Filters in main-app, add the alias of your Auth filter like this:

Ah, ok, to me, this is unexpected behaviour i meant. I'm expecting it to work just by registering it on extendables because it is in the extendables' scope and extendables' routing. Meaning, if i add new filter to extendables or updating the filter's class name, the changes will be breaking changes by default.

Also, just to make sure, is this just me, or is current implementation is expected to work like this (manually register package's filter to the main project)? Because, if that's how current implementation works, then @MGatner's project wont... work...? Do i miss something?

Thank you!

MGatner commented 3 years ago

I didn't follow your question at the end, but I can tell you that my linked Menus library works exactly as specified - I'm already using it in a few projects.

I will try to update the User Guide with module filter registration info, and hopefully that will clear this up for you.

paulbalandan commented 3 years ago

Step 2 in my suggestion is not unexpected. Currently, the framework has no way of telling what alias you will be using for a particular Filters class unless you register it yourself. Unless a Filters class has an $alias property that the framework scans and subsequently uses, developers adding new Filters should add the alias manually in their Config\Filters class.

@MGatner's library works because he manually registered the alias menu as reference to the MenusFilter class https://github.com/tattersoftware/codeigniter4-menus/blob/2937bed80f72bfa48da7dba9fed96b65a7369d1a/src/Config/Filters.php#L9

chez14 commented 3 years ago

Hi @paulbalandan,

So, just to clarify, I just did what @MGatner do, on f0c64f89d67b37b6b56f0373d55f8ec9ec690ecd, and i still need to do the second step you mentioned earlier? Forget this one, sorry. I'll check other files while waiting.

I'm sorry, I'm really lost right now. I think I'll just wait for someone to update the documentation and write the 'workaround' on the project's README for the time being.

Thank you both for the support! Really appreciate it.

kenjis commented 2 years ago

@chez14 @MGatner I'm not sure this is enough, but I sent a PR: #5281