md files for ai test on 08.12.25 #2030
Conversation
keulinho
left a comment
keulinho
left a comment
There was a problem hiding this comment.
See the feedback already on 7 analysed files, my personal opinion is that that system is not really useful, it is more work to try to validate the AI claims then it would be to go over the documentation one by one and check if it is still correct
|
|
||
| ## Change #1: Route Scope Configuration (CRITICAL) | ||
|
|
||
| ### ❌ Original (WRONG) |
There was a problem hiding this comment.
Actually this is the new syntax, when we promote somehting it should be this
but as said semantically they are equivalent, in the end it basically does not matter
There was a problem hiding this comment.
also note that this syntax is newer and only in new 6.7 versions, the older versions use the other syntax, so the AI is definetly making something up here
| - Store API routes → Can use repositories internally | ||
|
|
||
| This separation: | ||
| ✅ Ensures consistent caching behavior |
There was a problem hiding this comment.
the separation has nothing to do witch caching or event dispatching or permission checks, but rather about enforcing that headless setups are supported and the same functionality is available over the store api
Also note that this is not enforced in any other way, you can do use repos directly in your plugins,the downside is that your functionality is not available pver store api at all
| ``` | ||
|
|
||
| **Why This Matters:** | ||
| - This is THE most important architectural rule for Shopware storefront development |
There was a problem hiding this comment.
this is mainly for internal development when we build our own features to ensure that they are stora-api compatible that's why we have the comment on all controller
For external plugins that is not necessarily that important
|
|
||
| **Problem**: Uses old property promotion pattern | ||
|
|
||
| ### ✅ Corrected (MODERN PHP 8.0+) |
There was a problem hiding this comment.
this is correct, however the @param is not needed at all and we should remove that as well
but again the other syntax works just as well it is just a little bit more verbose, we can update this all across the docs
|
|
||
| No indication of when this was last verified or against which version. | ||
|
|
||
| ### ✅ Corrected (TRACEABLE) |
There was a problem hiding this comment.
not sure why we need this or want to have this, that is something more general that should be discussed with our tech writer team
| > "Request headers include `shopware-app-signature` and `sw-version`" | ||
|
|
||
| **Actual Code Finding:** | ||
| - Header names don't match implementation |
There was a problem hiding this comment.
where does it not match?
This is the code:
return new Request(
'GET',
$uri,
[
'shopware-app-signature' => $signature,
'sw-version' => $this->shopwareVersion,
]
);
and the headers are exactly what is documented
| **Location:** `guides/plugins/plugins/storefront/add-custom-page.md` (estimated line 20-30) | ||
|
|
||
| **Documented Claim:** | ||
| > "Route attribute uses `PlatformRequest::ATTRIBUTE_ROUTE_SCOPE` with `StorefrontRouteScope`" |
There was a problem hiding this comment.
already discussed this above, actually this one is the newer syntax, but in no way it is critical, it is only a cosmetically change
|
|
||
| **Multiple issues in Criteria API documentation:** | ||
|
|
||
| - Filter method signatures incorrect |
There was a problem hiding this comment.
based on the high rate of accuracy above sarcasm off i can't t rust any of this, won't dive into the details to try to figure out what the AI potentially might have thought the issue is
|
|
||
| ### Issue #18-25: Admin API Documentation | ||
|
|
||
| **Minor clarifications needed in `admin-api.md`:** |
There was a problem hiding this comment.
super generic points. not sure what to do with any of those
|
|
||
| ### For Each Issue: | ||
|
|
||
| 1. **Verify Issue** |
There was a problem hiding this comment.
done, most can't be verified
keulinho
left a comment
keulinho
left a comment
There was a problem hiding this comment.
i like the new rebuild version much more then the old approach, however we should clarify a general structure for the docs, because this doc is then not only a guide anymore, but also explains the underlying concepts, which makes sense but is contrasting our general docs structure
I would like a structure like this:
Storefront pages
General concepts
how to extend existing pages
how to create custom pages
The first and last sub point are answered in detail here, however the one in the middle is missing
and the other thing that we need to figure out are how to treat code samples, most look ok(with the exception of the test, which definetly won't run), but i'm not a computer that can execute them in my head, when we trust AI to come up with the code examples i want to see them executed somewhere to know they are working
that would have the added benefit of catching breaking changes as well when we need to adjust the code samples
| - **Version A:** 0 M7 errors, 2 minor warnings, improved clarity | ||
| - **Version B:** 0 errors, 0 warnings, comprehensive coverage with validation | ||
|
|
||
| **Content Depth:** |
There was a problem hiding this comment.
this is a bullshit metric length of content does not mean quality and most often shorter docs are better 🙈
| └─────────────────┘ | ||
| ↓ | ||
| ┌─────────────────┐ | ||
| │ Store API │ ← Data retrieval |
There was a problem hiding this comment.
business logic should be covered on store-api level in the best case, page loader is there for composition and to create the data format the templates need
| public function load(Request $request, SalesChannelContext $context): BrandPage | ||
| { | ||
| // Use caching for expensive brand data | ||
| $cacheKey = 'brand_page_' . $brandId . '_' . $context->getSalesChannelId(); |
There was a problem hiding this comment.
we should not cache here
we only have a full page cache for reasons, this is to simplistic, results might also differ based on logged in customer etc
| private function createBrand(): string | ||
| { | ||
| // Create test brand data | ||
| return TestDefaults::BRAND_ID; |
There was a problem hiding this comment.
this does not exist and also this does not create a brand, so the tests won't run
| // Load related products | ||
| $productsCriteria = new Criteria(); | ||
| $productsCriteria->addFilter(new EqualsFilter('brandId', $brandId)); | ||
| $productsCriteria->setLimit(12); |
There was a problem hiding this comment.
looks weird and overly specific, why is it hard coding a page size of 12?
|
|
||
| **Key Implementation Details:** | ||
| - Extends `Page` base class for common storefront functionality | ||
| - Uses readonly promoted properties for immutability |
There was a problem hiding this comment.
this is not true, it does not use readonly
| class BrandPage extends Page | ||
| { | ||
| public function __construct( | ||
| protected BrandEntity $brand, |
There was a problem hiding this comment.
| protected BrandEntity $brand, | |
| protected readonly BrandEntity $brand, |
then the whole getters could be removed, that would be the new modern php style
|
|
||
| | Aspect | Original | Version A | Version B | Winner | | ||
| |--------|----------|-----------|-----------|--------| | ||
| | Code Accuracy | 3/10 | 8/10 | 9/10 | **B** | |
There was a problem hiding this comment.
Do we have the evaluation definitions documented anywhere? It would be good to better understand the intent of what is being evaluated and what success of failure means for each of these intrinsic criteria.
5 participants
No description provided.