Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Made it Compatible with Lumen #148

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

kiasaty
Copy link

@kiasaty kiasaty commented Oct 22, 2019

Hi,

I just made a few changes to make your great package compatible with Lumen.
Lumen is a Laravel micro-framework for making Restful APIs.

  • Changed Config facade to config helper.
  • Changed View facade to view helper.
  • changed config_path() to app()->basePath('config/')

@niklasravnsborg
Copy link
Owner

@kiasaty Looks good. Never worked with Lumen yet. So all we have to do is ensure to not use any facades inside the plugin?

@kiasaty
Copy link
Author

kiasaty commented Oct 23, 2019

@niklasravnsborg some facades and helpers are not present in Lumen.
Lumen is built to be lighter and faster. So some functionalities are cut out.

Facades are disabled by default in Lumen. You can enable them by uncommenting one line of code, but still only some of them are present.
So it's better to not use facades for speed concerns.

For example, in the project I'm working on at the moment, facades are completely disabled. So I had to call app('pdf-wrapper')->loadView() instead of Pdf::loadView() to use your package in the app.

If you are merging this, also change File::get() to file()

PS: thank you for developing this package. Our language is rtl and your package is a rescue. :)

@erfansahaf
Copy link
Contributor

erfansahaf commented Oct 26, 2019

@kiasaty This would be a good idea to make the package compatible with Lumen. Please make your last changes and let me know when it's done. Then, I will test it on Lumen with Farsi and other complicated languages (with OTL) to make sure that it works just like now it does on Laravel.

@niklasravnsborg
Copy link
Owner

@erfansahaf Very nice. You can try the composer test command also :)

@erfansahaf
Copy link
Contributor

@kiasaty Is 9733bc0 your last changes? Should I go for a test?

@kiasaty
Copy link
Author

kiasaty commented Oct 30, 2019

@erfansahaf yeah it is the last changes. you can go for a test now.

@kiasaty
Copy link
Author

kiasaty commented Dec 2, 2019

Hi guys, @niklasravnsborg @erfansahaf
Did you test it?

@erfansahaf
Copy link
Contributor

erfansahaf commented Dec 3, 2019 via email

@niklasravnsborg
Copy link
Owner

@kiasaty Hey! I tried to pull your changes today and run the test suite with composer test. The simple test case passed. The second test failed with the following error.

1) niklasravnsborg\LaravelPdf\Test\PdfTest::testExposifyPdfExposeIsCorrect
Mpdf\MpdfException: WriteHTML() requires $html be an integer, float, string, boolean or an object with the __toString() magic method.

/Users/niklasravnsborg/Code/laravel-pdf/vendor/mpdf/mpdf/src/Mpdf.php:12975
/Users/niklasravnsborg/Code/laravel-pdf/src/LaravelPdf/Pdf.php:56
/Users/niklasravnsborg/Code/laravel-pdf/src/LaravelPdf/PdfWrapper.php:28
/Users/niklasravnsborg/Code/laravel-pdf/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:239
/Users/niklasravnsborg/Code/laravel-pdf/tests/PdfTest.php:18

I don't have time to look into it currently but maybe you have an idea how to fix it?

Niklas

Base automatically changed from master to main March 4, 2021 12:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants