Twill: Refactor usage of Facade aliases, Facades and Helper functions

[stevanpavlovic]

What do you guys think about refactoring the code base by replacing Facade aliases with Facade FQCN? It's really hard to track those definitions (even if you are using smart IDEs like PHPStorm).
Meaning:
use Event; should be rewritten to use Illuminate\Support\Facades\Event;

Since this package represents a core system, I think everything should be defined explicitly and usage of Facades and helper functions is generally, in my opinion, the way to go with concrete implementations (derived applications) and not something to be used in a source system. A source system should be as precise and as tightly coupled as possible. So another proposition from me:

File::makeDirectory($twillCustomBlocksPath) should be rewritten to $this->fileSystem->makeDirectory($twillCustomBlocksPath) and paying attention to proper injection of $this->fileSystem through a constructor.

Another one:

collect(...) should be rewritten to new Collection(...), same goes for config(...) => $this->configRepository->get(...) // $this->config->get(...), etc.

This way we can achieve having a more stable, cleaner and performant code. This way we would also have real dependency injections.

Any thoughts?

[yusufkandemir]

These things depend on the project. In a small Laravel project, these generally increases the development time while not providing many advantages. But as you have said

Since this package represents a core system

, it's better to replace facades with dependency injection and get rid of global helpers. Here is a quote from When to Use Facades (Laravel Docs)

When building a third-party package that interacts with Laravel, it's better to inject Laravel contracts instead of using facades. Since packages are built outside of Laravel itself, you will not have access to Laravel's facade testing helpers.

If we are not going to replace, I think we need to replace the facade aliases with facade FQCNs at least. So I agree with you.

[yanhao-li]

@stevanpavlovic @yusufkandemir
Great suggestions, and I'm with you on that.
Please feel free to create new PR if find anywhere could be improved!

[stevanpavlovic]

@yusufkandemir that's exactly what I wanted to achieve! Thanks for a more detailed explanation!
@yanhao-li thanks for your support!

[stevanpavlovic]

So we could split this enhancement into several smaller tasks in order to maintain stability and have a better control over what is changed over the time:

1. Replace all imports of alias facade class names with Facade FQCNs (for example use Event; should be rewritten to use Illuminate\Support\Facades\Event;)
2. Implement injection of Laravel contracts instead of using facades
3. Replace helper functions usages with actual service methods' calls in the all places where it's possible (for example, it could be possible to do this in some actual class and it could not be possible in the config files)

What do you guys think? @yanhao-li @yusufkandemir

[yanhao-li]

Hey @stevanpavlovic ,

Thanks for splitting the task into small steps, it makes the goal much more clear.

As the requirement is lucid and doesn't need much insights on Twill's codebase. I have labeled this issue with good first issue, so anyone who are interested in and want to dive deep into the codebase, could help to contribute!

[stevanpavlovic]

There are some places which need a further refactoring because there are impossible to implement DI completely. Those places are helper functions (example: s3Endpoint), models and traits. I would propose the refactoring of those functionalities by putting them in some better organized service classes. I would postpone this refactoring for the moment when we have some tests and typehinted methods and classes, so I consider this one as done.

Feel free to ask any questions, add comments or request changes.

[ifox]

Merged and available in 1.2.2, thank you for your hard work on this @stevanpavlovic!

As discussed, I refactored some of the changes that would have warranted a breaking change release, especially the constructors of Twill's main classes: ModuleController and ModuleRepository. Even for a breaking release, I wouldn't want to have Twill users forced to inject so many Laravel core dependencies in their own controllers and repositories, so we should discuss another approach to improving this, and plan ahead for a major release in the long term if we want to propose core API changes.

Thanks again, that's an amazing start to improve Twill's codebase overall quality!