Refactored resource loading logic out of the resources #576

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@webmozart

I think that the current resource classes are very hard to understand (I spent a lot of time figuring out what they do). IMO that's because they have too many responsibilities:

  • they represent resources
  • they load resources (in case of DirectoryResource and CoalescingDirectoryResource)

For this reason, I refactored the resource loading logic out of the resource classes. DirectoryResource and CoalescingDirectoryResource are deprecated now. Their getFresh() and getContent() methods don't seem to be used at all.

If we need resource collections with collective getFresh() and getContent() methods, I suggest adding a ResourceCollection class instead and use that instead of arrays. So far, that didn't seem necessary.

Before:

$directory = new DirectoryResource('/views', '.twig');
$file = new FileResource('/bundle/views/layout.html.twig');

$am->setLoader('am', $loader);
$am->addResource($directory, 'twig');
$am->addResource($file, 'twig');

After:

$loader = new DirectoryLoader();
$file = new FileResource('/bundle/views/layout.html.twig');

$am->setLoader('am', $loader);

foreach ($loader->load('/views', '.twig') as $entry) {
    $am->addResource($entry, 'twig');
}
$am->addResource($file, 'twig');

For me, this is much easier to grasp.

Once the deprecation phase is over and DirectoryResource removed, the code for iterating directories can be removed from CachedFormulaLoader. Again, this makes sense in my opinion. All other formula loaders take resources which correspond to files, so passing directory resources here is confusing for me.

Before:

$directory = new DirectoryResource('/views', '.twig');
$formulaLoader = new TwigFormulaLoader($twig);
$cachedLoader = new CachedFormulaLoader($loader, $cache);

// doesn't work
$formulaLoader->load($directory);

// works (huh?)
$cachedLoader->load($directory);

After:

$directoryLoader = new DirectoryLoader();
$formulaLoader = new TwigFormulaLoader($twig);
$cachedLoader = new CachedFormulaLoader($loader, $cache);

$files = $directoryLoader->load('/views', '.twig');

foreach ($files as $file) {
    $formulaLoader->load($file);
    // or
    $cachedLoader->load($file);
}

Since I don't know what the backwards compatibility policy is for Assetic, I left all existing code in as deprecated and did not introduce any BC breaks.

@webmozart

This comment has been minimized.

Show comment Hide comment
@webmozart

webmozart Feb 25, 2014

Btw another simplification would be to replace ResourceInterface by FileInterface. As far as I see, all elementary resources (Twig templates, config files etc.) are files anyway (and the developer who reads the code (me) doesn't spend unnecessary time figuring out why that abstraction is needed, even though it isn't :) ). What do you think?

Btw another simplification would be to replace ResourceInterface by FileInterface. As far as I see, all elementary resources (Twig templates, config files etc.) are files anyway (and the developer who reads the code (me) doesn't spend unnecessary time figuring out why that abstraction is needed, even though it isn't :) ). What do you think?

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Feb 25, 2014

Collaborator

The issue I see here is that it will become impossible to register the routing resources in the AsseticBundle after this change, because the Symfony resource system suffers from the same issue (which is logical as it was the inspiration used when implementing them in Assetic). See symfony/symfony#7230

Collaborator

stof commented Feb 25, 2014

The issue I see here is that it will become impossible to register the routing resources in the AsseticBundle after this change, because the Symfony resource system suffers from the same issue (which is logical as it was the inspiration used when implementing them in Assetic). See symfony/symfony#7230

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Feb 25, 2014

Collaborator

and the DirectoryResource is necessary to detect the case where you add a new file in a directory, so that it is parsed as well

Collaborator

stof commented Feb 25, 2014

and the DirectoryResource is necessary to detect the case where you add a new file in a directory, so that it is parsed as well

@mpdude

This comment has been minimized.

Show comment Hide comment
@mpdude

mpdude Apr 9, 2014

Contributor

Hi guys,

merely by accident I just found this issue. I have opened a few PRs/issues regarding the Resource-related concepts in Symfony a while ago which have caused me quite some headache:

There is also a PR for BC-compatible minimal changes in Symfony (symfony/symfony#7781) that would allow to introduce service-based Resource validation and also support non-filesystem-based or other custom Resources. That is currently designed to happen completely in userland. That might also help in fixing symfony/AsseticBundle#168.

My point is that we might find a way to design "assetic resources" however they work best for Assetic and then have bridging code in AsseticBundle that comes into play when need to check those "assetic resources" for freshness (-> rebuilding routes for the AsseticController).

Does that sound as if I could help?

Contributor

mpdude commented Apr 9, 2014

Hi guys,

merely by accident I just found this issue. I have opened a few PRs/issues regarding the Resource-related concepts in Symfony a while ago which have caused me quite some headache:

There is also a PR for BC-compatible minimal changes in Symfony (symfony/symfony#7781) that would allow to introduce service-based Resource validation and also support non-filesystem-based or other custom Resources. That is currently designed to happen completely in userland. That might also help in fixing symfony/AsseticBundle#168.

My point is that we might find a way to design "assetic resources" however they work best for Assetic and then have bridging code in AsseticBundle that comes into play when need to check those "assetic resources" for freshness (-> rebuilding routes for the AsseticController).

Does that sound as if I could help?

@mpdude

This comment has been minimized.

Show comment Hide comment
@mpdude

mpdude Oct 20, 2014

Contributor

bump

Contributor

mpdude commented Oct 20, 2014

bump

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Oct 30, 2014

Collaborator

@mpdude The issue is that designing Assetic resources so that they are checked for freshness externally will not allow us to bridge them to Symfony resources after that (which is our real use case for Assetic resources in the first place), given that Symfony resources need to be self-contained.

So the Symfony architecture change is a prerequisite for us being able to solve thing here, unless we kill the integration in Symfony resources entirely (which is a pretty bad idea)

Collaborator

stof commented Oct 30, 2014

@mpdude The issue is that designing Assetic resources so that they are checked for freshness externally will not allow us to bridge them to Symfony resources after that (which is our real use case for Assetic resources in the first place), given that Symfony resources need to be self-contained.

So the Symfony architecture change is a prerequisite for us being able to solve thing here, unless we kill the integration in Symfony resources entirely (which is a pretty bad idea)

@mpdude

This comment has been minimized.

Show comment Hide comment
@mpdude

mpdude Oct 30, 2014

Contributor

Yes, that Symfony change is what I'm after.

I think symfony/symfony#7781 contains the minimal changes I'd need to come up with a solution that can live outside of the core. The other two issues mentioned above also target at that but might be too broad to be easily discussed.

Anyway, I'd like if you two guys could have a look at symfony/symfony#7781. If you don't see how this might help here, let me know.

Contributor

mpdude commented Oct 30, 2014

Yes, that Symfony change is what I'm after.

I think symfony/symfony#7781 contains the minimal changes I'd need to come up with a solution that can live outside of the core. The other two issues mentioned above also target at that but might be too broad to be easily discussed.

Anyway, I'd like if you two guys could have a look at symfony/symfony#7781. If you don't see how this might help here, let me know.

@webmozart webmozart closed this Dec 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment