- Refactoring to Laravel 8 Class Model Factories. #5190
Conversation
|
Hi @AbdullahFaqeir and @ghermans, This PR is huge, it will take time as I am taking pull also. Meanwhile, I am opening the test case. |
|
Hi @AbdullahFaqeir, My local test cases are failing, please have a look,
I exited in the middle because already CI is failing. |
|
I'll go over the test cases again and will update the PR accordingly |
|
@devansh-webkul not only the local tests are failing, even on Github they are failing |
- update codeception driver for laravel.
There was a problem hiding this comment.
There are some changes we need to do,
- As going through all the files, it looks like your prettier or code beautifier has removed all the spaces. Please go through it, I mentioned in some.
- Mostly new lines are added before and opening classes.
- Traits imports can be done in single line rather than multiple uses.
packages/Webkul/Attribute/src/Database/Factories/AttributeFamilyFactory.php
Outdated
Show resolved
Hide resolved
packages/Webkul/Attribute/src/Database/Factories/AttributeOptionFactory.php
Outdated
Show resolved
Hide resolved
packages/Webkul/BookingProduct/src/Providers/BookingProductServiceProvider.php
Outdated
Show resolved
Hide resolved
packages/Webkul/BookingProduct/src/Providers/BookingProductServiceProvider.php
Outdated
Show resolved
Hide resolved
packages/Webkul/Category/src/Providers/CategoryServiceProvider.php
Outdated
Show resolved
Hide resolved
|
Hi just summarised here also because files are 92, Meanwhile, test cases are still failing. I am re-running again for you but in my local, it's also failing. |
- Code reformat.
- Remove codeception/module-laravel5 as it was causing trigger testing to fail since it's outdated and we already upgraded factories to laravel 8.
packages/Webkul/BookingProduct/src/Providers/BookingProductServiceProvider.php
Outdated
Show resolved
Hide resolved
packages/Webkul/BookingProduct/src/Providers/BookingProductServiceProvider.php
Outdated
Show resolved
Hide resolved
|
@devansh-webkul is everything okay? |
|
@AbdullahFaqeir please check the feedback that @devansh-webkul provided 4 hours ago |
I did, handled them in my last commit. |
|
Hi @AbdullahFaqeir, Most of the files, PSR-2 standard missing. Maybe your extension automatically doing it. |
|
Any update on this? |
you mean @AbdullahFaqeir hahahah, I'm using phpStorm, can you help me regarding the code style thingy? |
|
@devansh-webkul I just check the whole system against PSR2 using PHP_CodeSniffer, and seems like the whole system is missing PSR-2 Standards!
The output is infinitely long. |
|
This is 99 files and I already commented on most of the files. Just go through all the comments, this will help you. |
Most of the files are following PSR-2 Standards. Just have a look at the files, there are some files in which changes are just styled the code. |
Okay, I'll go over them file by file, my changed files (99) file and I'll update the PR |
|
I can help you with this. I will comment where changes are needed after you commit. |
- Fixing typo.
|
@devansh-webkul I have reviewed all 99 files each by it self and fixed the new lines and the extra spaces in all of them. |
|
I am checking, will let you know about this. |
There was a problem hiding this comment.
Hi @AbdullahFaqeir,
Just check the unresolved conversation, for your convenience, I just checked all the files thoroughly.
Only a few changes left.
|
Hi @AbdullahFaqeir,
|
|
@devansh-webkul I've fixed both the CS and the conflict. |
|
Okay! Now everything seems fine to me. I am opening the test case and checking my local instance also. |
Great, looking forward for the merge. <3 |
|
Just checking in my local. I Will let you know. |
@AbdullahFaqeir Thanks for the contribution. 🚀 |
|
I'mm honoured that I managed to help 🚀 🚀 🚀. |
|
Thank you for your contribution @AbdullahFaqeir |
You're most welcome, glad I managed to help ❤️ |
Issue Reference
#5165
Description
Dropping laravel legacy factories and refactor all models to use laravel 8 class factories, and some other code refactoring.
How To Test This?
Run tests.