-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add new structure code #35
Conversation
3b56d3d
to
c38108d
Compare
89d3d93
to
399d5eb
Compare
please could you review?
well let's discuss about it :) |
5efc5d0
to
b713dec
Compare
any plan to review? @tim-kos |
@kvz should be good to create a new branch |
do you consider review this MR yet @tim-kos |
Hey @eerison i am so sorry to keep you waiting. Tim has been swamped, and likely will be for the foreseeable future so I'm now asking internally if someone else on the team can take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this work @eerison! I haven't done too much PHP lately but I'll do my best to review. I like that there's a parent interface for the Assembly resource classes here, from what I remember that is very useful with dependency injection in frameworks like Laravel.
IMO there should be a way to provide Assembly instructions as an array. In the Java SDK we have a similar imperative API for building Assemblies as in this PR, but there is not really any other option in Java…PHP is much more dynamic and we should take advantage of that. I think it would also be important for adoption of the new version, if users have to rewrite all their Assembly instructions they may not find the time for that very soon.
Maybe it could look something like this?
$assembly = Assembly::fromParams([
'steps' => [
'resize' => ['robot' => '/image/resize', ...],
],
'redirect_url' => 'https://server001.my-app.com/',
]);
$assembly->addFilePath('/PATH/TO/FILE.jpg');
src/Factory/AuthFactory.php
Outdated
{ | ||
public static function create(string $key, string $secret, DateTimeInterface $expires = null): AuthInterface | ||
{ | ||
return new Auth($key, $secret, $expires); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the purpose of all the Factory classes. I feel like just having new Auth($key, $secret)
in the code is more readable and writable than AuthFactory::create($key, $secret)
. Is there some benefit that I am missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @goto-bus-stop first of all, thank you so much for your time to try to review my PR
regarding about your questions
from what I remember that is very useful with dependency injection in frameworks like Laravel.
Exactly ❤️ (DIP
and ISP
)
IMO there should be a way to provide Assembly instructions as an array.
Well use array in this point I'm not sure if it's the best approach
1 - you need to validate the data that is inputed in your method, because you can receive any data.
2 - who has using this lib always need to check the structure to pass the right "key"
IMO is better to use Object
instead of array
Other point is, using array is more complicated you set default parameters like templates
, credentials
and so on ...
if you see here, always I need to pass template
but this parameter never change.
in my example you can use this like
$parameter = new Parameter([... steps], 'templateId');
//or
$parameter->setTemplateId('1234')
this way you can pass your resource
already configured in yours services, you just need to pass your steps like
just a example
class YourService
{
private $assembly;
private $assemblyResourceService;
public function __controller(AssemblyInterface $assembly, AssemblyResourceServiceInterface $assemblyResourceService)
{
$this->assembly = $assembly;
$this->assemblyResourceService = $assemblyResourceService;
}
public function your_method()
{
$step1 = StepFactory::create('resize', [
'robot' => '/image/resize',
'width' => 200,
'height' => 100,
]);
$assembly = $this->assembly;
// and here there are parameters like templateId, notifyUrl, allowStepsOverride and so on
$assembly->getParameter()
->addStep($step1)
->addStep($step2)
///....
;
$assembly->addFilePath('your_path.img');
$this->assemblyResourceService->create($assembly);
}
}
I don't really understand the purpose of all the Factory classes. I feel like just having new Auth($key, $secret) in the code is more readable and writable than AuthFactory::create($key, $secret). Is there some benefit that I am missing?
I did it just to avoid you coupling code like new Step();
and the others factories like AuthFactory
, AssemblyFactory
etc ... I guess it will be used in standalone application only, not in applications that use symfony, laravel and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, that code snippets clears some things up for me about how the AssemblyInterface is intended to be used. (I was still thinking that you'd create them manually inside controllers or service classes rather than injecting them together with the resource->create()
one.)
Your point about objects over arrays makes sense, but I also don't think we really need to validate the array input … the Transloadit API will reject invalid inputs, so people who still use the arrays would just run into an error later down the line. What I'd hope for is that we could recommend using the new object style but have the array style as a migration path from 3.x. (so users only have to update their function calls and not also rewrite the templates etc)
I did it just to avoid you coupling code like new Step();
That does feel like unnecessary abstraction IMHO…or is that what all modern PHP libraries do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @goto-bus-stop
Thank you, that code snippets clears some things up for me about how the AssemblyInterface is intended to be used. (I was still thinking that you'd create them manually inside controllers or service classes rather than injecting them together with the
resource->create()
one.)Your point about objects over arrays makes sense, but I also don't think we really need to validate the array input … the Transloadit API will reject invalid inputs, so people who still use the arrays would just run into an error later down the line. What I'd hope for is that we could recommend using the new object style but have the array style as a migration path from 3.x. (so users only have to update their function calls and not also rewrite the templates etc)
I got it your point ....
I'll see some way to accept object
and array
to accept migrate the old version, and I'll post here :)
in the version 4.1.0
I will add Deprecation that will be removed in version 5
That does feel like unnecessary abstraction IMHO…or is that what all modern PHP libraries do?
I guess it's ok to remove those factories, because usually devs uses some framework to do Dependency injection
Perhaps similar to the Node SDK v3, |
Sorry for the long delay here @eerison ! I went on vacation shortly after you had submitted the PR, but never reported back here. Either way, thank you so much for all the hard work you put into this! And thank you @goto-bus-stop for taking over here! |
Hi @goto-bus-stop have you seen my comment above? |
c1d87a1
to
abbf5ed
Compare
if we going to add a code compatible with the currently code, then I prefer keep the current code and add please could you review again? |
9c8cc9d
to
e5eb64e
Compare
Just to be clear, there is no Break Change in this Pull request ! |
Hi Erison, @goto-bus-stop is on holidays right now. I wonder if @juliangruber can have a look in the meantime? |
Closing this pull request, reason added in the issue related with this pr |
Fix: #34
Usage
Create an assembly
Get an assembly
Cancel an assembly
To test local
Note:
Please after this merge pass, don't create a new release, I'll do a new Pull request creating the doc
using material for Doc, like fake-php
cc: @kvz and @tim-kos