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

Fix compatibility with Lumen #1043

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Fix compatibility with Lumen #1043

merged 3 commits into from
Sep 11, 2020

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Sep 8, 2020

Summary

Fixes #1042

Got broken by #1026

TL;DR: can't use anything in the Illuminate\Foundation namespace
without safety check, as it doesn't exist in Lumen.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@mfn
Copy link
Collaborator Author

mfn commented Sep 8, 2020

the static analysis stuff can be ignored, will be fixed in another PR

@mfn
Copy link
Collaborator Author

mfn commented Sep 8, 2020

the static analysis stuff can be ignored, will be fixed in another PR

=> #1044

Copy link
Contributor

@DannyvdSluijs DannyvdSluijs left a comment

Choose a reason for hiding this comment

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

LGTM. IMHO a small helper function isLaravel(): bool might be a nice improvement which would provide the context the comment is now doing.

@mfn
Copy link
Collaborator Author

mfn commented Sep 8, 2020

LGTM. IMHO a small helper function isLaravel(): bool might be a nice improvement which would provide the context the comment is now doing.

Thanks, added!

@pataar
Copy link
Contributor

pataar commented Sep 9, 2020

Tested this, and it seems to work. 😄

mfn added 3 commits September 9, 2020 09:54
Fixes barryvdh#1042

Got broken by barryvdh#1026

TL;DR: can't use anything in the `Illuminate\Foundation` namespace
without safety check, as it doesn't exist in Lumen.
Copy link
Collaborator Author

@mfn mfn left a comment

Choose a reason for hiding this comment

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

@pataar thanks for confirming

@mfn mfn requested a review from barryvdh September 9, 2020 07:55
@mfn
Copy link
Collaborator Author

mfn commented Sep 10, 2020

ping @barryvdh IMHO this is good to merge (at least, it's the best idea I could come up with, maybe you have a better one 😬)

@barryvdh
Copy link
Owner

Hah sorry, to be honest I feel like the helper is a bit overkill in this case.
But I can't remember what this PhpEngine is for. It seems hacky, so I'd rather fix it at the core. Why can't we use the normal view engine? I'm guessing there was a reason but not sure what.

@mfn
Copy link
Collaborator Author

mfn commented Sep 10, 2020

Why can't we use the normal view engine?

Yeah about that 😅

  • Apparently some use a custom template engine (e.g. twig) which remove the support for PHP templates in Laravel
  • This library though requires PHP templates, as it's how all the commands generate the PHP stub source code
  • And then there are users of this library who override the PHP templates with their own extensions, so getting rid of PHP templates altogether can break their flow

It's kind of a catch-22

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.

Error: Class 'Illuminate\Foundation\Application' not found in file (When using Lumen)
4 participants