- Download Diff

Summary:

[Lunr.Locator] Core: add PSR-11 support

Review Request #827 — Created April 26, 2022 and submitted June 24, 2022, 4:38 p.m.

Information
Owner:	smillernl
Repository:	Lunr.Locator
Branch:	feature/core/psr-11
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Core: add PSR-11 support

Testing Done

Issues

0
0
17
1
All Issues: 18

Description	From	Last Updated
var_export() can produce multi-line output. I'm not sure that's what we want to use here. Conceptually we already have a ...	pprkut	May 5, 2022, 3:39 p.m.
Failed to locate object for identifier '$id'! seems a bit nicer	pprkut	May 5, 2022, 3:39 p.m.
We can be more strict here I think. We're allowed to add the string typehint in child classes, we just ...	pprkut	May 5, 2022, 3:39 p.m.
No ContainerException here? Seems inconsistent	pprkut	May 5, 2022, 3:39 p.m.
I'm not sure we need the redirect. locate() is a protected method, so we can just inline it here. The ...	pprkut	May 5, 2022, 3:38 p.m.
Not possible to instantiate '{$reflection->name}' is a bit clearer I think.	pprkut	May 5, 2022, 3:38 p.m.
We can do better	pprkut	May 5, 2022, 3:38 p.m.
Here too	pprkut	May 5, 2022, 3:38 p.m.
It doesn't though	pprkut	May 5, 2022, 3:38 p.m.
It doesn't do that either	pprkut	May 5, 2022, 3:38 p.m.
dead code	pprkut	May 5, 2022, 3:38 p.m.
dead code	pprkut	May 5, 2022, 3:38 p.m.
semi-dead code	pprkut	May 5, 2022, 3:38 p.m.
We already did the registry check when we enter the method. Executing has() does it again. I prefer the old ...	pprkut	June 9, 2022, 8:35 a.m.
Similar here. has() and get() do redundant checks . Outside the class this may be acceptable, but inside we want ...	pprkut	June 9, 2022, 8:35 a.m.
I think we can just replace this with a call to load_recipe(), and then check the cache. But also have ...	pprkut	June 17, 2022, 3:01 p.m.
composer.lock probably shouldn't be changed, considering there's no change to composer.json either	pprkut	June 23, 2022, 2:52 p.m.
We also need a cache check before load_recipe(), then we have the same logic as get(), just without actually trying ...	pprkut	June 23, 2022, 2:52 p.m.

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)

Show all issues

var_export() can produce multi-line output. I'm not sure that's what we want to use here.

Conceptually we already have a backtrace attached to the exception, we should be able to read or at least discern the value from that.

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)

Show all issues

Failed to locate object for identifier '$id'! seems a bit nicer

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)

Show all issues

We can be more strict here I think. We're allowed to add the string typehint in child classes, we just can't remove it.

Same for the get() method.

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)
Show all issues
```
No ContainerException here? Seems inconsistent
```

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)

Show all issues

I'm not sure we need the redirect. locate() is a protected method, so we can just inline it here. The only thing I'm not sure about is how potential strict typing would affect get_parameters().

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 1)

Show all issues

Not possible to instantiate '{$reflection->name}' is a bit clearer I think.

src/Lunr/Core/Exceptions/ContainerException.php (Diff revision 1)
Show all issues
```
We can do better
```
src/Lunr/Core/Exceptions/NotFoundException.php (Diff revision 1)
Show all issues
```
Here too
```
src/Lunr/Core/Tests/ConfigServiceLocatorHasTest.php (Diff revision 1)
Show all issues
```
It doesn't though
```
src/Lunr/Core/Tests/ConfigServiceLocatorHasTest.php (Diff revision 1)
Show all issues
```
It doesn't do that either
```
src/Lunr/Core/Tests/ConfigServiceLocatorLocateTest.php (Diff revision 1)
Show all issues
```
dead code
```
src/Lunr/Core/Tests/ConfigServiceLocatorLocateTest.php (Diff revision 1)
Show all issues
```
dead code
```
src/Lunr/Core/Tests/ConfigServiceLocatorLocateTest.php (Diff revision 1)
Show all issues
```
semi-dead code
```

Diff:

Revision 2 (+594 -311)

Show changes

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Core/ConfigServiceLocator.php
	src/Lunr/Core/Exceptions/ContainerException.php
	src/Lunr/Core/Exceptions/NotFoundException.php
	src/Lunr/Core/Tests/ConfigServiceLocatorBaseTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorGetInstanceTest.php
	5 more

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 2)

Show all issues

We already did the registry check when we enter the method. Executing has() does it again. I prefer the old version of the code.

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 2)

Show all issues

Similar here. has() and get() do redundant checks . Outside the class this may be acceptable, but inside we want a more optimized version and don't execute useless code.

pprkut June 8, 2022, 2:05 p.m.

I think we can just drop the call to has(). In the flow, if we have a string value that isn't meant to be used as a string value it's most definitely a missing locator recipe and we want to error out.
The previous code was entirely built around not erroring out anywhere. It's either that or erroring out everywhere, but not some weird middle ground. So

$processed_params[] = $this->get($value);

should be good enough

Diff:

Revision 3 (+604 -320)

Show changes

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Core/ConfigServiceLocator.php
	src/Lunr/Core/Exceptions/ContainerException.php
	src/Lunr/Core/Exceptions/NotFoundException.php
	src/Lunr/Core/Tests/ConfigServiceLocatorBaseTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorGetInstanceTest.php
	5 more

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 3)

Show all issues

I think we can just replace this with a call to load_recipe(), and then check the cache.
But also have to check the cache before calling load_recipe()

smillernl June 15, 2022, 4:33 p.m.

But you don't always want to load the file. You just want to know it's available right?

pprkut June 16, 2022, 3:42 p.m.

It's an optimization. Situations where you call has() and get() in sequence are more likely than has() returning TRUE and not calling get() after.

So we generally save one call to stream_resolve_include_path().

But especially checking the cache would be important. Accessing the filesystem is slow and we don't load a recipe more than once. Also, just because the file exists doesn't mean the information in the file matches. That's why it's important to also check the cache after the recipe is loaded from the file system.

Diff:

Revision 4 (+248 -169)

Show changes

	composer.lock
	src/Lunr/Core/ConfigServiceLocator.php
	src/Lunr/Core/Exceptions/ContainerException.php
	src/Lunr/Core/Exceptions/NotFoundException.php
	src/Lunr/Core/Tests/ConfigServiceLocatorBaseTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorGetInstanceTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorLocateTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorHasTest.php
	3 more

composer.lock (Diff revision 4)

Show all issues

composer.lock probably shouldn't be changed, considering there's no change to composer.json either

src/Lunr/Core/ConfigServiceLocator.php (Diff revision 4)

Show all issues

We also need a cache check before load_recipe(), then we have the same logic as get(), just without actually trying to instantiate anything.

Diff:

Revision 5 (+250 -153)

Show changes

	src/Lunr/Core/ConfigServiceLocator.php
	src/Lunr/Core/Exceptions/ContainerException.php
	src/Lunr/Core/Exceptions/NotFoundException.php
	src/Lunr/Core/Tests/ConfigServiceLocatorBaseTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorGetInstanceTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorLocateTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorHasTest.php
	src/Lunr/Core/Tests/ConfigServiceLocatorSupportTest.php
	2 more

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.24

[Lunr.Locator] Core: add PSR-11 support

Diff:

Diff:

Diff:

Diff:

Status: Closed (submitted)