Web requests "isConsoleRequest" still returns true

[myleshyson]

Noticed today that when making a request, modules/plugins still think the request is a console request because of the way yii handles getting that value.

For example:

// ViewControllerTest.php
$repsonse = $this->actingAs($this->user)
        ->http('post', UrlHelper::url('actions/ufh-calendar/view/move-event'))
        ->setBody([
            'id' => $event->id,
            'oldStartDate' => Json::encode($event->startDate->toDateTime()),
            'newStartDate' => Json::encode(($event->startDate->copy()->addDays(1)->toDateTime()))
        ])
        ->send();
// returns a 404 response

// CalendarModule.php
if (\Craft::$app->request->isConsoleRequest) {
    $this->controllerNamespace = __NAMESPACE__ . '\\console\\controllers'; // <------- uses this, because isConsoleReuest is true
} else {
    $this->controllerNamespace = __NAMESPACE__ . '\\controllers';
}

I think the only way around this is to re-bootstrap craft after the initial bootstrap, so that all plugins/modules end up using the correct namespace. On bootstrap, "_isConsoleRequest" is null. Because of that When getting that property, yii actually looks at the PHP_SAPI constant instead, which is set to "cli". That happens so early on I don't see any way of preventing it.

I took the code from craftcms/vendor/craftcms/cms/src/test/CraftConnector.php:84 and added it to my own base test case. Afterwards my test was able to find the action like expected. What do you think about adding that as a part of the bootstrap process?

Also wondering if there should be a setting that allows tests to "re-bootstrap" if they want to actually use the console request and not the web requests.

[myleshyson]

For a little more clarity here's the manifestation of the issue:

[markhuot]

Can you try again with dev-master please? I actually pushed a related fix the other day for exactly this.

The spot you’re logging is during Craft’s initial bootstrap. We make completely new requests for each get() call. https://github.com/markhuot/craft-pest/blob/8ce74103a3fd1fb4a9c097f79354c1f8cdb22ca1/src/http/requests/WebRequest.php#L130

[myleshyson]

yea it's still a problem unfortunately. It's because the modules/plugins are only bootstrapped once in the beggining, and when they're bootstrapped they're init functions are using the inital request object created in web.php...which has isConsoleRequest set to true.

Normally every request is going to re-bootstrap every single module/plugin. The way it's implemented now however that's not happening and it causes issues.

I'm confusing myself talking about it, so here's a really concrete example of what's happening!

// CalendarModule.php
public function init()
{
        \Craft::setAlias('@modules/ufh-calendar', __DIR__);

        parent::init();

        if (\Craft::$app->request->isConsoleRequest) {
            $this->controllerNamespace = __NAMESPACE__ . '\\console\\controllers';
        } else {
            $this->controllerNamespace = __NAMESPACE__ . '\\controllers';
        }
}

// ViewController.php
 public function actionTestAction()
    {
       return $this->asJson('Hi!');
    }

// ExampleTest.php
test('without re-bootstrapping modules/plugins', function() {
    $this->get('/actions/ufh-calendar/view/test-action')->assertOk();
});

test('with re-boostraping modules/plugins', function() {
    /** @var Module $module */
    foreach (Craft::$app->getModules() as $module) {
        $moduleClass = get_class($module);
        $moduleId = $module->id;

        if ($module instanceof PluginInterface) {
            $plugins = Craft::$app->getPlugins();

            // Follow the same error handling as Craft does natively.
            if (($info = $plugins->getStoredPluginInfo($moduleId)) === null) {
                throw new InvalidPluginException($moduleId);
            }

            $module = $plugins->createPlugin($moduleId, $info);
        } else {
            $module = new $moduleClass($moduleId, Craft::$app);
        }

        /** @var string|Module $moduleClass */
        /** @phpstan-var class-string<Module>|Module $moduleClass */
        $moduleClass::setInstance(
            $module
        );
        Craft::$app->setModule($moduleId, $module);
    }

    $this->get('/actions/ufh-calendar/view/test-action')->assertOk();
});

And then the test results.

What needs to happen is that bit of functionality re-bootstrapping modules/plugins should be run on every request, to fully mimick a normal craft request cycle. Otherwise every plugin/module out there is gonna have failed tests.

[markhuot]

Yup, I'm following now. Thanks for the test case!

Here's the rub though, flipped around, a lot of plugins aren't going to expect to bootstrap twice in the same memory space. I have a feeling we'll run in to issues there too… I can think of a few plugins that add event listeners but never remove them and if we re-init them then we'll end up with duplicate event handlers…

Nonetheless, let me get a new branch pushed and a failing test in there that we can iterate on.

I think a safer approach would be force the initial bootstrap to think it's a web request so everything bootstraps within the web environment.

[markhuot]

Hum… digging in to this more I'm actually very confused because we bootstrap the web app type so I'm not sure why the initial bootstrap thinks its a console request.

https://github.com/markhuot/craft-pest/blob/8ce74103a3fd1fb4a9c097f79354c1f8cdb22ca1/src/test/TestCase.php#L99

[markhuot]

Ahh, darn globals all over the place in Yii. They're checking PHP_SAPI === 'cli' initially. I'll have to find a place/way to override that…

https://github.com/yiisoft/yii2/blob/1d557360d544e1fa871b2bd2f9ce7daf6d8b674d/framework/base/Request.php#L39

[myleshyson]

Yea it's tricky. On the one hand it's weird to re-bootstrap everything since a request in this instance isn't firing off another process. On the other hand, there's could totally be valid cases for modules/plugins to check the request in their init fucntion and act based on what the request is. There's no middleware's in Yii so the module init is the next best thing depending on the context. In that case, even if the inital bootstrapping got it right isConsoleRequest = false, module/plugin developers still can't test if requests do everythging they're supposed to.

I don't actually think the init running again in the same memory space really matters. Each module/plugin will be getting passed the same exact configuration. They'll just also have access to the most up to date components in the service container which is important.

I was looking at Laravel to see how they do it, and they're actually bootstrapping everything in the kernel each time you call the request as well.

https://github.com/laravel/framework/blob/a709c26b8e37b20826e8ebe7fa14d29751fedcc5/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php#L535

Kinda wish you could boostrap everything like that in Craft. Would make this a little easier!

I don't think we need to boostrap the entire application each time, just the modules and plugins.

[markhuot]

So, if we were to re-run the init, what would change? On the second time through the plugin would see a proper request that would have the requesting page (from $this->get(‘page’)) and correctly know if this is a console request?

I’ll give it a try, but just looking at a few popular plug-ins like Neo, that means their fields will be added to the field list multiple times. Gql types too.

https://github.com/spicywebau/craft-neo/blob/0117df4a754b7fd3290ac93fb486eed51e183f66/src/Plugin.php#L121-L123

Best case scenario is that the field shows up twice in the UI. Worst case is that it starts throwing random errors around the CP.

Or, in Retour’s case it’ll double add event listeners for saving elements. That means every save will generate two redirects in the table because $event->isNew here will be true on each call,

https://github.com/nystudio107/craft-retour/blob/87a6ed8a29f55eb6438c4847fd74019234e77185/src/Retour.php#L361

All that said, I do think this is worth a try. But in the meantime I have this PR which flips the default around for plug-ins (at least). Need to look at modules next,

https://github.com/markhuot/craft-pest/pull/44

[myleshyson]

Dang yea you're right about the double event handling. Totally missed that. #44 should work fine for now but I can imagine that this is still something that should be solved eventually. I wonder on each request if you could stash the current Craft::$app somewhere, call TestCase::createApplication() (it would have to be made static), and then use that newly created app for the request. Then when the request is done do something like Craft::$app = $originalApp?

Also sorry for bombarding you with all these issues and prs. I just really dislike using codeception and really love using this plugin. Trying to bring stuff up as I come across testing various modules in our project. it feels like it's 90% there give or take a few things like this!

[markhuot]

I wonder on each request if you could stash the current Craft::$app somewhere, call TestCase::createApplication() (it would have to be made static), and then use that newly created app for the request. Then when the request is done do something like Craft::$app = $originalApp?

Yup, I've been down this path. Yii gets really upset when you try to Yii::createObject(Application::class) (pseudo-code) multiple times. There are some singletons in there (like the DB) that just can't deal with multiple instantiations. The one I could never work through is that as soon as you bootstrap the app a second time the DB connection hangs indefinitely. MySQL yells that the code is deadlocked, but it makes no sense because it deadlocks on queries and I can't figure out where the connection isn't being properly released.

Also sorry for bombarding you with all these issues and prs. I just really dislike using codeception and really love using this plugin. Trying to bring stuff up as I come across testing various modules in our project. it feels like it's 90% there give or take a few things like this!

Woo! Please keep em' coming. I've been using it mostly for front-end testing of my templates, but just added it in to a module I'm working on so your bugs are coming at exactly the right time. Thank you.

--

So, I think #44 gets us past the initial problem for plugins, but leaves modules still broken because they point to the console controller path. We can fix plugins easily because there's an EVENT_ for plugin initialization. But there's nothing for modules, because they boot through Yii's default mechanism (well before Plugins come in to play). Realizing there were no hooks I could grab on to for modules lead me to this abomination,

https://github.com/markhuot/craft-pest/blob/710f9718f64c293201b6bec7fcf0dcedbef77919/src/test/TestCase.php#L135-L152

I don't love it at all, but it works for both plugins and modules. It passes all the tests on 3.7 and 4.2… I really don't know…

[markhuot]

Okay, thought on this and really didn’t like mucking with the src via all those regular expressions. It felt much to brittle.

I revised the approach to take some inspiration from Mockery so I’m now leaning on Composer’s autoloader to muck around with the ENV. What’s happening is I’m basically subclassing craft\services\Config with my own custom subclass that allows me to return a modified config. I’m doing this for the app.web.php config (which most people don’t have set in their project anyway). This allows me to override the Application class and gain access to the bootstrap process much earlier than any event or plugin would have access to.

https://github.com/markhuot/craft-pest/blob/3afbd5d0f2ef50df00b67a6b100ac50224c934e6/src/bootstrap/bootstrap.php#L72-L77

Unlike Mockery, instead of just inserting a blank class in place of craft/services/Config I actually move the native config over to a new overrides namespace and then insert my custom subclass at the original namespace. Effectively allowing me complete control over the Config class. All this rigamarole so that I can set this before anything init()s.

https://github.com/markhuot/craft-pest/blob/3afbd5d0f2ef50df00b67a6b100ac50224c934e6/src/web/Application.php#L9

I feel much better about this. Once all the tests pass in the Action I’ll get this merged and another -alpha* release tagged.

[markhuot]

1.0.0-alpha9 includes this fix.