affordablemobiles / GServerlessSupportLaravel

Run Laravel on Google Serverless
MIT License
26 stars 17 forks source link

Laravel 6? #48

Closed ProgrammerZ closed 3 years ago

ProgrammerZ commented 5 years ago

Great pack, but do you plan to upgrade to Laravel 6 in the nearest future?

iamacarpet commented 5 years ago

It is something we will be doing, as we’ll need to upgrade once 5.5 LTS ends it’s security support in Aug 2020, but I’ll be honest and say it isn’t on the short term roadmap.

We’ve got a couple of other high priority projects that are taking precedence and as our developers have only just upgraded to L5.5 (thanks Google for the massive delay in php72 going GA), they can’t timetable any more upgrades for a while either.

Happy to accept PRs for a new branch to support Laravel 6 though, if you get to it before us.

Hopefully most of the code can be reused, but we haven’t even had time to investigate yet.

iamacarpet commented 5 years ago

@matthewnessworthy you mentioned not much was incompatible with L6.0 in one of your PRs, which suggested you'd given it a try.

Can you give us any idea what else you think will need fixing or improving to support it properly?

matthewnessworthy commented 5 years ago

So far the issues i've run into appear to be a minor copy-paste issue with the error handler, the Laravel 5.6 change to logging. Apart from that most things appear to work fine on Laravel 6.0. As i run into issues i'll continue to submit PRs.

iamacarpet commented 4 years ago

@matthewnessworthy , I noticed there was a fix for Laravel 7 you'd made (Exception -> Throwable) that didn't make the latest round of PRs:

https://github.com/matthewnessworthy/GaeSupportLaravel/commit/c738c395603aa9b7a2d97877cbc71d2c33f77b22#diff-58703262750a8e7aab1a30c464661fd7R5

It isn't in it's own commit, so I can't easily pull it and retain your attribution.

Happy to accept it into the Laravel 6 branch if you think it's also compatible there too (or is it Laravel 7 only?).

matthewnessworthy commented 4 years ago

@iamacarpet it should be compatible for Laravel 6 & 7, so feel free to cherry-pick it in, or just grab the code. Not terribly worried about attribution on small commits like this.

iamacarpet commented 4 years ago

@matthewnessworthy , thanks, pulled that in this afternoon.

As a general question to everyone, how many of you are using this library with Laravel 6 or higher?

And for those that are, how much of the library are you guys using?

I'd like to try and build a list of things we know are working, e.g.

Would love to hear your feedback.

@jliversidge and @dstretton FYI

flexgrip commented 4 years ago

I'm relatively certain that logging and error reporting are working. The blade pre-compiler wasn't working on L7. It was telling me things like /var/www/resources/views/home.blade.php doesn't exist When it does.

I had the queue driver (cloud tasks) working manually with some spotty routes I had setup. But I'll try to see if I can get it to work from this package.

I've read the docs a few times. I'm still not even clear what the view compiling does. We get a lot of traffic and lots of page loads and the usage seems normal in the GAE dashboard. GAE is so cheap. I just wonder what it would help with. Would it reduce memory usage?

iamacarpet commented 4 years ago

Thanks @flexgrip ,

The view compiler just saves it having to do the translation from Blade templating language (into the PHP intemediary code that actually runs) on each instance during a cold boot, reducing the p90+ page load times, as the translator / compiler is quite CPU intensive (probably would help with memory too, but can't confirm - it is more aimed at page load time reduction).

Since instances are fairly short lived, for a site with a lot of traffic, over time it can make quite a considerable difference to these edge cases. The temp folder stuff that Laravel ships with (and does work fine on the GAE instances by writing into /tmp) was really designed for long lived machines that rarely die, with the cache only invalidated as new templates are deployed, which definately isn't the case for App Engine.

We'll only intend to jump to L6 until the next LTS is released, so fixing it for L7 won't be on our radar for now.

I will take a look into the queue driver stuff when we update the project internally that is using it - thanks for the heads up!

flexgrip commented 4 years ago

Ok cool. Thanks @iamacarpet. I'm going to try to get it enabled again.

After upgrading to L7, I thought this package was causing build issues on appengine. Turned out it was other dependencies. I'm going to test stackdriver, queuedriver and the view compiler. I already had manual implementations of both of those drivers. So I'll switch over to the ones from this package.

flexgrip commented 4 years ago

Has anyone using this package had issues with instances running out of memory? I don't think its this package, it might be one of the dependencies. But on gae standard, I keep seeing Exceeded soft memory limit of 512 MB with 521 MB after servicing 95 requests total. Consider setting a larger instance class in app.yaml.

I tried to dig around the instances to see what was going on. So I made a little endpoint that would return the contents of the /tmp directory. Because I realized either the runtime can take up space in memory or /tmp can put files in memory there. The tmp directory wasn't taking up much disk space. But then I noticed this:

/tmp/batch-daemon-failure - 0MB
/tmp/batch-daemon-failure/. - 0MB
/tmp/batch-daemon-failure/.. - 0MB
/tmp/batch-daemon-failure/failed-items-21 - 34.3MB
/tmp/batch-daemon-failure/failed-items-22 - 57.41MB
/tmp/batch-daemon-failure/failed-items-23 - 37.12MB
/tmp/batch-daemon-failure/failed-items-24 - 44.12MB

So those batch-daemon-failure files are starting to take up quite a bit of space. I grep'd around for it and found a reference to it in the google-cloud-core php package.

Anyway, that leads me back to here. The only package I run that imports that cloud core package from google is this one :/

I don't know if this problem existed in the versions prior to L6. I dunno. I'm out of ideas. At this point, I'm trying to see if there's an env var to turn off the batch daemon thingy (I don't even really know what it does).

Any info would be appreciated. Maybe that package isn't needed or something?

iamacarpet commented 3 years ago

We're way later to the party than I'd have hoped, but Laravel 6.0 is now going to be our default going forward.

First release that we'll be actively using tagged today in a new branch (have coalesced the two branches as they had some drift), which we'll update with any fixes as and when we discover any problems.

As you can see, I'd ideally like to be targeting the php74 runtime, it currently isn't working.

Hopefully we'll be a bit more on the ball when Laravel 9 LTS drops in September.