Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: WIP Laravel Octane support #833

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jangaraev
Copy link

  1. binding in service provider changed to a way proposed by Laravel Octane in case if we detect it
  2. introduced a listener to hook into Octane's RequestReceived event

Refs: #780

1. binding in service provider changed to a way proposed by Laravel Octane in case if we detect it
2. introduced a listener to hook into Octane's RequestReceived event

Refs: mcamara#780
@jangaraev
Copy link
Author

Let's start with that. I will test this on the staging platform of my live project.

My main point to consider is can we completely switch to bind() in the service provider instead of permanently checking for Octane's presence. I'm worrying it affects performance badly for non-Octane environments.

Also did nothing with unit tests.

@jangaraev
Copy link
Author

Just a small update.

Tested this code

  • on the live project with Octane enabled and everything works super-fine
  • locally on the regular environment with no Octane --> fine as well

@sylouuu
Copy link

sylouuu commented Jul 22, 2022

Any news @jangaraev to undraft this? :)

@jangaraev
Copy link
Author

hi. sorry for the silence. I was on vacation last 2 weeks.

The code in PR is good actually, tested this in projects I work on. My idea was to provide proper tests for that, but I have a quite limited experience on testing Octane. so I'm stuck at the moment.

I need to check how to test things in Laravel in default and octane enabled environments.

@didix16
Copy link

didix16 commented Sep 7, 2022

Any news about merging PR and updating the localization package?

Thanks

Comment on lines +62 to +72
if ($this->runningInOctane()) {
// the 3rd parameter is important to be passed to 'bind()'
// otherwise the package's instance will be instantiated every time
// you reference it and it won't get proper data for 'serialized translatable routes'
// class variable, this will make impossible to use translatable routes properly
// but overall the package will still work stable except generating the same URLs
// for translatable routes independently of locale
$this->app->bind(LaravelLocalization::class, $fn, true);
} else {
$this->app->singleton(LaravelLocalization::class, $fn);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

singleton() is just a bind() with true as a third parameter, so this check shouldn't make any difference.

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Container/Container.php#L388

@fomenko-oi
Copy link

Hello! Any news from here?
Because there are problems with Laravel Ocatane now. When you use 'localeCookieRedirect', 'localizationRedirect' middlewares, you have got infinitive redirect cycle

@taai
Copy link

taai commented Aug 13, 2023

@jangaraev Any news about the PR? 😉

@korridor
Copy link

Any updates on this pull request? I would really love Octane support. :)

@UtkuDalmaz
Copy link

UtkuDalmaz commented Sep 5, 2023

any update please???? @mcamara

@korridor
Copy link

korridor commented Sep 5, 2023

I tried the solution and it didn't solve the problem for me. I also looked into the code of the solution, and it seems a bit hacky, but I'm not that deep into this plugin.

Since this broke my production environment and I needed a multi-language solution fast, I uninstalled this plugin and used this instead: https://github.com/codezero-be/laravel-localized-routes

Worked with Octane out of the box and the migration process was surprisingly easy. I think it is also nice that route:cache works normally there.

@UtkuDalmaz
Copy link

I tried the solution and it didn't solve the problem for me. I also looked into the code of the solution, and it seems a bit hacky, but I'm not that deep into this plugin.

Since this broke my production environment and I needed a multi-language solution fast, I uninstalled this plugin and used this instead: https://github.com/codezero-be/laravel-localized-routes

Worked with Octane out of the box and the migration process was surprisingly easy. I think it is also nice that route:cache works normally there.

perfect thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants