Bee-Lab / bowerphp

A PHP implementation of bower :bird:
MIT License
467 stars 37 forks source link

Config as singleton #64

Closed piotrooo closed 3 months ago

piotrooo commented 9 years ago

I think config should be represent as singleton pattern. Now is passed to all objects but should be one instance of object, with global access.

garak commented 9 years ago

I don't know... as you mention, singleton is a kind of global, and this recalls the bad old times of PHP. Anyway, I'm open to any suggestions: if you can summarise pros and cons of both approaches, we can discuss it

julkue commented 9 years ago

Well I don't think singletons are good ideas.

piotrooo commented 9 years ago

Yeep maybe singleton is nod good idea, but passing configs for all object are not good too. Maybe some factories for config for package and main bower file.

piotrooo commented 9 years ago

I thinking about try to implements global configs. I still believe that a good solution will be singleton with reload function.

Why I think is will be good approach? Main config file (bower.json) should be globally accessed without need for passing or creating it in each object. Package config files (/.bower.json) should be globally access too.

In this objects should be method to reload configs.

Passing configs for each object is a little awkward, such an approach obscures methods usage and generates another level of complication.

@garak why you thing is bad old time? :) @julmot why?

garak commented 9 years ago

Global was (and still is) bad because makes hard to understand where variables are coming from. On the other side, passing objects gives maximum control, and it's a pattern known as "inversion of control"

piotrooo commented 9 years ago

You explanations convinces me. But I think is needed to extract creating config, filesystem etc from command object and pass to command by IoC in Appication getDefaultCommands method. This approach is strictly IoC convention.

garak commented 9 years ago

I agree that commands need a refactoring. I was thinking to introduce a small dependency container (maybe Pimple) to do that

piotrooo commented 9 years ago

I try to implement pimple ioc container only for update command, tests wasn't working but this commit is only proof of concept, so if you have any suggestions we can talk about it.