Conversation
There was a problem hiding this comment.
I'm still not entirely comfortable with AbstractResource.
For me, Resource is not a Sitekit resource but a universal resource container.
The obectType concept is also general, as it allows the type of resource to be identified depending on where it comes from.
$dataBag is the content of the loaded resource.
What I didn't want is for us to have various resource classes again that have to be derived from Resource (now ResourceAbstract).
I think that's no longer necessary now that we have the features.
But maybe I haven't quite understood yet how you want to use AbstractResource.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==============================================
- Coverage 100.00% 82.03% -17.97%
- Complexity 198 262 +64
==============================================
Files 16 44 +28
Lines 706 807 +101
==============================================
- Hits 706 662 -44
- Misses 0 145 +145 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I removed |
|
Yes, I think that's fine. Are there any tests coming up? |
|
Perhaps SiteKitResource could already be used here? |
|
Ok, I think I need some feedback before going any further because I'm not sure If Im moving in the right direction. I added some basic resource features (located under I think there's much to discuss here which we should probably do in person. |
Current state overviewThe ResourceView system is implemented as discussed. Here's a short recap:
Quick example // $this->resourceViewManager is injected using DI
// create a resource view of a given resource $resource
$resourceView = $this->resourceViewManager->forResource($resource);
// access teaser data
$teaserFeature = $resourceView->get(TeaserFeature::class);So the basic services for this resource view system is already implemented, but feel free to suggest changes. Next stepsI think this bundle should provide a bunch of basic Here are the
Then there are some more features that the AI suggested, but are technically not needed yet:
Some bundles will also need additional features. For example, the I also couldn't help but implement some basic models like The implementation of both the ResourceFeatures as well as the models is very important to get right from the start and there's much room for debate here, so feel free to share you thoughts. @sitepark-schaeper @Schleuse @hyra-soe @sitepark-asc |
There was a problem hiding this comment.
My initial thoughts were:
-
What is the advantage of separating
ResourceViewFactoryandResourceViewManager? In my opinion the factory can just as well cache the views. -
Shouldn't the
ResourceViewBatchContributorextendResourceViewContributor? I cannot imagine a case where a class would want to implement the batch-contributor but not the "normal" one. -
It's weird that the
ResourceViewFactorydelegates toResourceViewBatchContributors via two separate arrays ofResourcesandResourceViewBuilders. Maybe wrap them in aResourceContributionBatchor something? -
I'm not sure about the name "Contributor". I know this is a draft, but it did throw me off.
ResourceViewEnricher?ResourceFeatureFactory? maybe evenResourceViewBuilderDecorator? (I know this is technically not a decorator..)
But now that I've played around with this I think I've got slightly more profound feedback:
I found implementing contributors to be somewhat awkward. They have to process the resource's data twice and even though the second time it should be valid they still have to handle the case where it isn't:
class ExampleLinkContributor implements ResourceViewContributor
{
#[\Override]
public function supports(Resource $r): bool
{
return $this->getLinkData($r) !== false;
}
#[\Override]
public function contribute(Resource $r, ResourceViewBuilder $b): void
{
[$url, $label, $isExternal] = $this->getLinkData($r)
?: throw new \Exception('invalid data');
$b->add(
LinkFeature::class,
new LinkFeature(
new Link(
url: $url,
label: $label,
isExternal: $isExternal,
),
),
);
}
/** @return list{string, string, bool}|false */
private function getLinkData(Resource $r): array|false
{
$link = $r->data->getArray('link');
$url = $link['url'] ?? null;
$label = $link['label'] ?? null;
$isExternal = $link['external'] ?? false;
if (!is_string($url) || !is_string($label) || !is_bool($isExternal)) {
return false;
}
return [$url, $label, $isExternal];
}
}This is especially annoying since PHP has no checked exceptions and it's hard to know/enforce where and which may be thrown. Take, for example, a contributor that uses a database:
class ExampleDbContributor implements ResourceViewContributor, ResourceViewBatchContributor
{
#[\Override]
public function supports(Resource $r): bool
{
return $this->isAppropriateResource($r)
&& $this->hasCorrectDataInDb($r); // might throw
}
/**
* @param list<Resource> $resources
* @param array<string, ResourceViewBuilder> $builders
*/
#[\Override]
public function contributeBatch(array $resources, array $builders): void
{
$queryBuilder = new QueryBuilder();
foreach ($resources as $resource) {
if (!$this->isAppropriateResource($resource)) {
throw new \Exception('invalid resource');
}
$queryBuilder->add($resource);
}
$links = $this->createAllFromDb($queryBuilder->build()); // can throw
foreach ($links as $id => $link) {
$builders[$id]->add(
LinkFeature::class,
fn() => new LinkFeature(
$this->fetchFrom($link), // can throw
),
);
}
}
// ...
}Now, the fetchFrom may be exaggerated, but I hope you can see that there may be a lot of places where exceptions could be thrown. And this is not only irritating for the implementation:
public function printLink(Resource $resource): void
{
try {
$view = $this->manager->forResource($resource);
} catch (\Exception $exception) {
echo 'invalid resource';
return;
}
try {
$link = $view->get(LinkFeature::class);
} catch (MissingFeatureException $exception) {
echo 'resource has no link';
return;
} catch (\Exception $exception) {
echo 'could not resolve resource data';
return;
}
echo "<a href=\"{$link->link->url}\">{$link->link->label}</a>";
}I feel like we can remove the supports check:
class ExampleLinkContributor implements ResourceViewContributor
{
#[\Override]
public function tryContribute(Resource $r, ResourceViewBuilder $b): void
{
$link = $r->data->getArray('link');
$url = $link['url'] ?? null;
$label = $link['label'] ?? null;
$isExternal = $link['external'] ?? false;
if (!is_string($url) || !is_string($label) || !is_bool($isExternal)) {
// alternatively return false but it does not seem to be
// relevant to know whether a contributor actually contributed.
return;
}
$link = new Link(url: $url, label: $label, isExternal: $isExternal);
$b->add(LinkFeature::class, new LinkFeature($link));
}
}This also improves the ResourceViewFactory:
public function createViews(array $resources): array
{
$builders = [];
foreach ($resources as $r) {
$builders[$r->id] = new ResourceViewBuilder();
}
foreach ($this->contributors as $contributor) {
- $supportedResources = [];
- $buildersForSupportedResources = [];
- foreach ($resources as $r) {
- if ($contributor->supports($r)) {
- $supportedResources[] = $r;
- $buildersForSupportedResources[$r->id] = $builders[$r->id];
- }
- }
- if (empty($supportedResources)) {
- continue;
- }
if ($contributor instanceof ResourceViewBatchContributor) {
- $contributor->contributeBatch($supportedResources, $buildersForSupportedResources);
+ $contributor->contributeBatch($resources, $builders);
} else {
- foreach ($supportedResources as $r) {
- $contributor->contribute($r, $buildersForSupportedResources[$r->id]);
+ foreach ($resources as $r) {
+ $contributor->tryContribute($r, $builders[$r->id]);
}
}
}(on a side note, we could also think about providing a separate BatchAwareViewFactory with this behaviour and make the current one just foreach ($resources as $r) { $result[$r->id] = $this->createView($r); } for when the batch loading isn't required)
What we could do, is to require contributors and the lazy-loading closures to return false/null instead of throwing (or catch them and then return null/false). We would not need the try-catches and could just do this:
$link = $manager->forResource($resource)->tryGet(LinkFeature::class)
?? throw new \Exception('resource has no link');An issue might be, that we cannot differentiate between
- the resource is invalid
- the resource has no link
- the factory has no link-contributor
- the link-contributor failed
- the lazy-loading failed
That probably is irrelevant at runtime, but might make it harder to set this up or track down bugs.
The "loading-from-database"-example also revealed a couple things to me:
If I have my resource data in a database or on another server, I probably would want the contributor(s) to lazy-load. However, the actual options to achieve that seem lackluster:
class ExampleDbContributor implements ResourceViewBatchContributor
{
/**
* @param list<Resource> $resources
* @param array<string, ResourceViewBuilder> $builders
*/
#[\Override]
public function tryContributeBatch(array $resources, array $builders): void
{
// either load everything lazily but requiring a db call each time
for ($i = count($resources) - 1; $i >= 0; $i--) {
$resource = $resources[$i];
$builder = $builders[$i];
$builder->add(
LinkFeature::class,
fn () => new LinkFeature($this->fetchLinkFromDb($resource)),
);
$builder->add(
SeoFeature::class,
fn () => new SeoFeature($this->fetchSeoFromDb($resource)),
);
// ...
}
// this also has the issue of data possibly changing in the db in
// between resolving
}
/**
* @param list<Resource> $resources
* @param array<string, ResourceViewBuilder> $builders
*/
#[\Override]
public function tryContributeBatch(array $resources, array $builders): void
{
// or always load everything all at once.
// this may be feasible or even ideal, but that heavily depends on the
// scenario in which it is used
$results = $this->fetchFromDb($resources);
foreach ($results as $result) {
$builder = $builders[$result->id];
$builder->add(LinkFeature::class, new LinkFeature($result->link));
$builder->add(SeoFeature::class, new SeoFeature($result->seo));
// ...
}
}
}Regardless of the second option being improvable or not, the first one is certainly inferior. Which begs the question: What good is the lazy-loading feature really? It seems to only be useful in the specific scenario where I have exactly one expensive feature, which has to be resolved individually for each resource regardless (like an atomic rest api). Other than that I currently find it to only complicate things (like the error handling mentioned earlier).
If we want this, we would have to be able to lazy-load more than one feature at a time. Taking the previous example further, imagine the data for both, the LinkFeature and the TeaserFeature requiring a db call, while the SeoFeature creation is inexpensive.
public function getSomeFeatures(): iterable
{
$viewA = $manager->forResource($this->resourceA);
yield $viewA->tryGet(LinkFeature::class); // load from db _once_
yield $viewA->tryGet(SeoFeature::class); // no db call required
yield $viewA->tryGet(TeaserFeature::class); // has already been loaded
// for these no db call should be made at all
$viewB = $manager->forResource($this->resourceB);
yield $viewB->tryGet(SeoFeature::class);
}Additionally, when batch-loading we have to be careful about:
- lazy-loading on a per-view basis as in the function above,
because we might want to iterate them and would still cause a lot of requests - lazy-loading all view-data of the entire batch,
because we might only need a very small portion of that data while potentially loading lots - not lazy-loading at all,
because we might not even need any db data
All of this depends heavily on the usecase, meaning we either have to offer the flexibility for users to implement this mechanism however they need it (which sounds hard to achieve elegantly) or find a way to propagate the required features ahead of loading them.
The more I look into this, the more it seems like we'd want to re-implement graphql here:
public function getSomeMoreFeatures(): iterable
{
// this could lazy-load the link- and teaser-features all together,
// but would not have to since we probably need that data regardless.
// (ignore the weird, imaginary method signature here..)
[$viewA, $viewB, $viewC] = $manager->queryForResources(
[$this->resourceA, [LinkFeature::class, SeoFeature::class]],
[$this->resourceB, [LinkFeature::class, TeaserFeature::class]],
[$this->resourceC, [TeaserFeature::class]],
);
}Depending on how we intend to use this, we may want to separate the Resource and their associated data.
A Resource would only need an id and language (or whatever uniquely identifies it) and we'd have a ResourceRepository which could hold (cache) their data. Here's a quick mock up:
class SimpleRepository implements ResourceRepository
{
/**
* @var WeakMap<
* string,
* array<class-string<ResourceFeature>, ResourceFeature|false>
* > $data
*/
private WeakMap $data;
/**
* @param list<list{string, list<class-string<ResourceFeature>>}> $query
* @return list<ResourceView>
*/
#[\Override]
public function view(array $query): array
{
$query = ResourceViewQuery::atomic($query);
return $this->resolveQuery($query);
}
/**
* @param list<string> $resources
* @param list<class-string<ResourceFeature>> $features
* @return list<ResourceView>
*/
#[\Override]
public function viewAll(array $resources, array $features): array
{
$query = ResourceViewQuery::builder()
->resources($resources)
->features($features);
return $this->resolveQuery($query);
}
/**
* @return list<ResourceView>
*/
private function resolveQuery(ResourceViewQuery $query): array
{
$context = new ResourceLoadingContext($query);
try {
foreach ($query->fields as $field) {
if (isset($this->data[$field->resource->id])) {
}
$features = $this->data[$field->resource->id];
if ($features === false) {
// we already tried to load this resource and failed
// ...
}
$feature = $features[$field->featureClass] ?? null;
if ($feature !== null) {
$query->resolve($field->resource->id, $feature);
continue;
}
$this->loaders->delegate($field, $context);
}
$this->cache($this->loaders->load($context));
return $query->unpack();
} catch (IncompletelyResolvedQueryException $exception) {
// ...
} catch (\Exception $exception) {
// ...
}
}
/**
* @param iterable<array<
* string,
* array<class-string<ResourceFeature>, ResourceFeature|false>
* >>
*/
private function cache(iterable $data): void
{
foreach ($data as $entry) {
foreach ($entry as $id => $values) {
$this->data[$id] = [...($this->data[$id] ?? []), ...$values];
}
}
}
}class DefaultResourceLoaders implements ResourceLoaders
{
/** @var array<class-string<ResourceFeature>, list<ResourceLoader>> $loaders */
private array $loaders;
#[\Override]
public function delegate(
ResourceViewQueryField $field,
ResourceLoadingContext $context,
): void {
$loaders = $this->loaders[$field->featureClass]
?? throw new \Exception('no appropriate loader');
foreach ($loaders as $loader) {
if ($loader->tryAddField($field, $context)) {
return;
}
}
throw new \Exception('no appropriate loader');
}
/**
* @return iterable<array<
* string,
* array<class-string<ResourceFeature>, ResourceFeature|false>
* >>
*/
#[\Override]
public function load(ResourceLoadingContext $context): iterable
{
foreach ($context->getActiveLoaders() as $loader) {
// should probably not be iterable as all items have to be consumed
yield $loader->load($context);
}
}
}class ExampleDbResourceLoader implements ResourceLoader
{
/** @var array<class-string<ResourceFeature>, list<ResourceLoader>> $loaders */
private array $loaders;
#[\Override]
public function tryAddField(
ResourceViewQueryField $field,
ResourceLoadingContext $context,
): bool {
if (!($field->resource instanceof SpecialResource)) {
return false;
}
$fields = $this->fieldsForFeature($field->featureClass);
if ($fields === false) {
return false;
}
$context->forLoader($this)->merge(
[
$field->resource->id => [
'fields' => $fields,
'features' => [$field->featureClass],
],
],
);
return true;
}
/**
* @return array<
* string,
* array<class-string<ResourceFeature>, ResourceFeature|false>
* >
*/
#[\Override]
public function load(ResourceLoadingContext $context): array
{
$data = $context->forLoader($this)->get();
$statements = ['BEGIN TRANSACTION;'];
foreach ($data as $id => $properties) {
$statements[] = 'SELECT id, ' . implode(', ', $properties['fields'])
. " FROM Resource WHERE id == '$id';";
}
$statements[] = 'COMMIT TRANSACTION;';
$resultSet = $this->db->executeStatements($statements);
$loaded = [];
foreach ($resultSet->rows() as $row) {
foreach ($data[$row->id]['features'] as $featureClass) {
$feature = $this->createFeature($featureClass, $row);
$loaded[$row->id][$featureClass] = $feature;
$context->loaded($row->id, $feature);
}
}
return $loaded;
}
/** @param class-string<ResourceFeature> $featureClass */
private function fieldsForFeature(string $featureClass): array
{
return match ($featureClass) {
LinkFeature::class => ['url', 'label', 'is_external'],
TeaserFeature::class => ['url', 'headline', 'image_url'],
default => false,
};
}
/**
* @template T of ResourceFeature
* @param class-string<T> $featureClass
* @return T
*/
private function createFeature(
string $featureClass,
SqlResultSetRow $row,
): ResourceFeature {
return match ($featureClass) {
LinkFeature::class => new LinkFeature(
url: $row->url,
label: $row->label,
external: $row->is_external,
),
TeaserFeature::class => new TeaserFeature(
url: $row->url,
headline: $row->headline,
image: $row->image_url,
),
default => throw new \Exception(
'encountered unexpected feature class',
),
};
}
}3 participants
Introduces resource views as described in issue #44
The suggested
ResourceResolverinterface was renamed toResourceViewContributor.Additionally, the following classes/features were added:
ResourceViewManager
The
ResourceViewManageracts as an high-level entry point for retrievingResourceViews. For now, this is pretty much just a wrapper forResourceViewFactory, which additionally caches theResourceViewobjects for each resource.Batched
ResourceViewcreation for better performanceResourceViewContributors can now optionally implement the interfaceResourceViewBatchContributorto define an optimized process for creating multiple resource views at once. In some cases, implementing this method can lead to significant performance gains, e.g. when a contributor has to make an expensive call to an API or a database.Here's a quick example:
Lazy-Loading resource features
ResourceViewnow supports lazy feature initialization. Instead of passing the already initialized feature objects to theResourceView, it can now also handle callables.So instead of doing this
we can do
I also added
ResourceView::preloadAll()as a way to resolve all features eagerly.However, there are also cons to lazy-loading. Here's what ChatGPT says about this implementation. 😄