Revisioning services?

[garak]

I'm just wondering why all service are defined in the prototype scope.
Some of them looks like just instances of your classes, so what is the advantage of calling them, instead of instantiating a plain object?
Other ones looks like just "plain" services, why should I get a new instance of them every time?

[jasvrcek]

That's a good question, I guess some of the services, such as jsvrcek_ics.utility.formatter should be in the container scope. There is definitely no reason to get a new instance of Formatter every time.

I made them all services just for consistency's sake. Because you need to inject Formatter into the Attendee object, it made sense to me to make that a service so the setup is handled for you. Having done that, it seemed that it would be inconsistent to get a Jsvrcek\ICS\Model\Relationship\Attendee object via the service container, but then have to instantiate Jsvrcek\ICS\Model\Description\Geo as a plain object. My thinking was that you should be able to get instances of all of the Model objects in the same way.

[tacman]

Hmm. It doesn't make sense to me that you'd inject a model via Dependency Injection. The only reason Attendee needs the formatter is for the getFormattedUri() call, which prepends "mailto:" to email addresses, or more precisely, to uris with an "@" that don't already have mailto: present.

In my recent PR, I did not make the models public in services.xml, and would propose removing them.

[tacman]

IF it's important to have a formatter already present in a new Attendee, and you want consistence, we could have model factories, I guess. Thoughts?

[jasvrcek]

@tacman I agree that injecting a model via DI is weird, and I am ok moving away from that. A model factory could make sense. Over time, what I have ended up doing in my own coding in practice is just manually creating instances of models when I need them, and creating a new Formatter instance when necessary. For example:

$attendee = new Attendee(new Formatter());

or

$recurrenceRule = new RecurrenceRule(new Formatter());

In some cases that could end up being done in a way that unnecessarily creates multiple Formatter instances, but whether that's a problem or not depends:

• In most cases you can avoid that by just doing $formatter = new Formatter(); and then passing $formatter into any model instances that need it.
• Even if you do end up in some edge case where you are forced to create extra instances of Formatter, is that even worth thinking about, and would fixing it add complexity just to over-optimizing something that will not have any real effect on the end product?

I think a model factory that might work something like this would be fine as well:

$recurrenceRule = $modelFactory->create(Attendee::class);

On the question of using a model factory vs manually instantiating models, I don't have strong feelings.

On the question of should we have models that require being injected with Formatter, I do think injecting Formatter is awkward. On the other hand I find it really useful, for example, to be able to get a string representation of a RecurrenceRule via RecurrenceRule::__toString. If we didn't inject Formatter into RecurrenceRule we would either have to make a service for converting models to strings, e.g. $convertToStringService->convert($recurrenceRule), or duplicate a function like Formatter::getFormattedUTCDateTime() in multiple places. So, every path seems to have drawbacks 🤷‍♂️