Skip to content
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

Update reflection based abstract factory documentation #136

Draft
wants to merge 3 commits into
base: 3.11.x
Choose a base branch
from

Conversation

settermjd
Copy link

Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

After working through the documentation to learn more about the Reflection-based Abstract Factor, I felt that the documentation could be refined to make it that much easier to read and maintain, as well as to extend the usage examples to make them more encompassing of different use cases. This PR replaces #134.

After working through the documentation to learn more about it, I felt
that the documentation could be refined to make it that much easier to
read, as well as to extend the usage examples to make them more
encompassing of different use cases.

Signed-off-by: Matthew Setter <[email protected]>
The intention of this change is to make the file easier to maintain,
rather than to have lines fit within a mandatory 72 - 80 char per/line
limit. Personally, this new approach makes content easier to move
around, to edit, and to remove as and when required, than arbitrary line
lengths that break sentences midway across one or more lines.

Signed-off-by: Matthew Setter <[email protected]>
@Ocramius
Copy link
Member

Note: IMO for 3.13.x

@settermjd
Copy link
Author

Note: IMO for 3.13.x

I'm pretty sure @froschdesign recommended 3.11. @froschdesign?

@Ocramius
Copy link
Member

Unless it's a bugfix, it should be landing in new minors in general :D

@froschdesign
Copy link
Member

Note: IMO for 3.13.x

As already mentioned: with the current state of documentation, in terms of quality and quantity, any change can be considered as a patch. No matter if something is updated or added.
I have no problem with a patch for the documentation being released as a minor version with other pull requests. But only a change to the documentation as a minor makes the release rather sad. Because nothing is changed on the user side after a Composer update.

@Ocramius
Copy link
Member

I'd do a patch release even for docs, which is just more noise tho? 🤔

In this case, I'd do 2 releases + 2 merge-ups

@froschdesign
Copy link
Member

I'd do a patch release even for docs, which is just more noise tho?

It would be much better if changes to documentation did not require releases at all. 😜

@Ocramius
Copy link
Member

I don't mind, honestly - we don't have enough noise from it for it to be a problem.

@settermjd
Copy link
Author

@Ocramius, if you don't mind either way, I'd prefer to leave it as is. On a different note, what can I do about the DCO check failing, as it relates to laminas bot?
Bildschirmfoto vom 2022-06-17 12-51-36
?

Just merge anyway?

@Ocramius
Copy link
Member

Yeah, DCO is useless corporateism, so it can be ignored on anything that isn't "world-changing tech", TBH.

@Ocramius
Copy link
Member

@froschdesign your call on where/how to merge/release

@Ocramius Ocramius added this to the 3.11.4 milestone Jun 29, 2022
Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

Good improvements, but the current usage example needs an update and clarification.

To alleviate this issue during development, laminas-servicemanager ships with the `ReflectionBasedAbstractFactory`, which provides a [reflection-based approach](https://www.php.net/manual/en/intro.reflection.php) to instantiation, resolving constructor dependencies to the relevant services.
The factory may be used as either an abstract factory or mapped to specific service names as a factory.

TIP: Mapping services to the factory is more explicit and performant.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the tip to the end of this section.

Suggested change
TIP: Mapping services to the factory is more explicit and performant.

Comment on lines +17 to +19
- If a service cannot be found for a given typehint, the factory will raise an exception detailing this.

WARNING: `$options` passed to the factory are ignored in all cases, as we cannot make assumptions about which argument(s) they might replace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list is introduced with "features" and "constraints" therefore this warning can be a normal list entry:

Suggested change
- If a service cannot be found for a given typehint, the factory will raise an exception detailing this.
WARNING: `$options` passed to the factory are ignored in all cases, as we cannot make assumptions about which argument(s) they might replace.
- If a service cannot be found for a given typehint, the factory will raise an exception detailing this.
- `$options` passed to the factory are ignored in all cases, as we cannot make assumptions about which argument(s) they might replace.


TIP: Mapping services to the factory is more explicit and performant.

The factory operates with the following constraints/features:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The positive first:

Suggested change
The factory operates with the following constraints/features:
The factory operates with the following features/constraints:

Comment on lines +26 to +28
### Using Laminas MVC

When using [Laminas MVC](https://docs.laminas.dev/mvc/):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package name for laminas-mvc is used most and is also the preferred way so far.
(If something refers to Mezzio, then the spelling with a capital initial is used: "Mezzio". The package names also follow the names used in Composer. Example: "mezzio-problem-details".)

Suggested change
### Using Laminas MVC
When using [Laminas MVC](https://docs.laminas.dev/mvc/):
### Using laminas-mvc
When using [laminas-mvc](https://docs.laminas.dev/mvc/):

Comment on lines +30 to +31
- Add the factory in the service manager's list of abstract factories.
- Use the `ReflectionBasedAbstractFactory` to instantiate the class.
Copy link
Member

@froschdesign froschdesign Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part and also the code example are wrong because you do not need both options to use ReflectionBasedAbstractFactory.

  • If the ReflectionBasedAbstractFactory is used as an abstract factory, then it will do the job for all services to which no explicit factory is mapped.
  • If the ReflectionBasedAbstractFactory is used as factory for a single service, then it will work only for this one service.

This means that we need two independent examples for the documentation, each with its own explanation!

Comment on lines +55 to +73
- Add the `ReflectionBasedAbstractFactory` to the `abstract_factories` array returned from the `getDependencies()` method.
- Use the `ReflectionBasedAbstractFactory` to instantiate the class, in the `factories` array returned from the `getDependencies()` method.

Once your dependencies have stabilized, we recommend writing a dedicated
factory, as reflection can introduce performance overhead; you may use the
[generate-factory-for-class console tool](console-tools.md#generate-factory-for-class)
to do so.
```php
use Laminas\ServiceManager\AbstractFactory\ReflectionBasedAbstractFactory;

public function getDependencies(): array
{
return [
'abstract_factories' => [
ReflectionBasedAbstractFactory::class,
],
'factories' => [
// Other factories...
Handler\HomePageHandler::class => ReflectionBasedAbstractFactory::class,
],
],
}
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above: this is wrong.

Comment on lines +21 to +22
Once your dependencies have stabilized and when performance is a requirement, we recommend writing a dedicated factory, as reflection can introduce performance overhead.
For example, you could use the [generate-factory-for-class console tool](console-tools.md#generate-factory-for-class) to do so.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest adding the tip here:

Suggested change
Once your dependencies have stabilized and when performance is a requirement, we recommend writing a dedicated factory, as reflection can introduce performance overhead.
For example, you could use the [generate-factory-for-class console tool](console-tools.md#generate-factory-for-class) to do so.
TIP: Once your dependencies have stabilized and when performance is a requirement, we recommend writing a dedicated factory, as reflection can introduce performance overhead.
For example, you could use the [generate-factory-for-class console tool](console-tools.md#generate-factory-for-class) to do so.

@boesing
Copy link
Member

boesing commented Apr 30, 2023

@settermjd @froschdesign I've also modified the documentation in #180

Should we try to finish this as well and then merge-up to 4.0.x?

@froschdesign
Copy link
Member

@boesing

Should we try to finish this as well and then merge-up to 4.0.x?

A good idea, because the entire documentation needs to be reworked for version 4.0.

@boesing boesing removed this from the 3.11.4 milestone May 14, 2023
@boesing boesing marked this pull request as draft May 14, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants