Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States TravisCarden
I've been taking a lot of rabbit trails... Running the UI app from outside the container is complicated. (Thanks to @bnjmnm for showing me how it's at least supposed to look when it works.) I've tried a few approaches that I hoped would limit the number of dependencies on the host, but it doesn't look like I'll get away without at least one--ZQuartz. In the meantime, I renamed the Cypress headless tests command to
ddev xb-cypress-run
and added addev xb-npm-ci
command to runnpm ci
to rebuild front-end assets when you make or pull down changes. - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Follow-ups:
- #19.1: 📌 [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (and I realized it had a blocker itself: ✨ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format — what if you want to use another FieldType? Active ), and then the 3 at the bottom of #17:
- 📌 [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria Active
- 📌 [PP-2] Remove Component config entity's add/edit form Postponed
-
📌
[PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type
Postponed
Bonus follow-ups: 📌 Component config entity should validate that the SDC actually (still) exists Active + 📌 [PP-2] Add ComponentAuditabilityTest Active .
I'll continue working on this MR on Monday. Now: time for blog posts :)
- 🇮🇳India secretsayan
Marking this again as Needs Review as PHPSTAN is already passing in the pipeline.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇦🇺Australia dpi Perth, Australia
In 2024, this should be an enum.
We also have an established backward compatibility pattern used by FileRepository (10.3 & 11.0) in 📌 Create enums for File exists options and deprecate consts RTBC which allows int|Enum, which we can adapt to string|Enum here.
- 🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
Turned patch #150 into a merge request for the new module (had to do a little convincing to get the patch to apply to the standalone module). Also, updated the deprecation info (I missedn #154, but the updates in there are obsolete anyway; I take it Forum will be doing its own deprecations, independently from core).
Still need to actually tests whether it (still) resolves the problem, although I suspect the latest reports of it not working are running into the standalone module being installed, while the patch applies to the core version.
- @eelkeblok opened merge request.
Is there any documentation about how `#config_target` should/can be used?
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Discussed #17 with @lauriii. His replies:
- Yes, but needs follow-up to devise a UX to allow existing
StaticPropSource
s using a previous field type+widget to update to the new one. - Yes
- Yes
He agreed with the 3 follow-ups proposed at the bottom of #17.
We also discussed #18, and he prefers the "alternatively" — so repurposed that 👍
P.S.: Note that this also blocks 📌 Create a component for testing form backend + frontend integration Active .
- Yes, but needs follow-up to devise a UX to allow existing
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Oh, and once this lands, 📌 [PP-1] Media Library integration Postponed becomes trivially unblocked: it'll only require:
- new configuration to allow declaring a preference for
image
vsmedia
(which oddly would not be possible through a newexperience_builder.settings.yml
config, only through a new config entity, because simple config cannot have module nor config dependencies) - updating
\Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage()
to respect that configuration
Those 2 things would be in an MR for that issue which I could start and then the front-end team could continue.
Alternatively, I could repurpose ✨ Hardcode image props to use the media widget temporarily Active for that, so that #3454173 can stay focused on the bigger picture, of making that media library widget actually work (well).
- new configuration to allow declaring a preference for
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
The
\Drupal\experience_builder\Form\ComponentEditForm
UX became simpler:Discussion with @lauriii
Yesterday morning, prior to writing #15, @lauriii and I met and discussed this problem space in detail.
The key outcomes of that discussion:
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
examples
array must not be empty, and XB will use the first one. ⇒ Removes the need for 25% of what theComponent
config entity provides. - @lauriii was heavily leaning towards "no UI" and "logic with an alter hook".
- Tricky: complex shapes, like
json-schema-definitions://experience_builder.module/image
— those would need a plugin system to provide the appropriate field type + field storage.
This issue: current MR state and current consequences
This issue proves it's possible to generate the default
field_type
,expression
andfield_widget
value for each prop. ⇒ Removes the need for 75% of what theComponent
config entity provides.This issue did implement it all as logic (i.e. with nothing stored as a config entity — see my comment #15 for why that turned out to be trickier than I thought when I created this issue), with a TODO for an alter hook.
As demonstrated in #16, this means the scope for 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed changes to only having to worry about matching existing field instances that can be adapted to match the SDC prop's enum values.
Questions for @lauriii prior to landing this MR
- Is it acceptable for the logic that determines which field type + widget to use to change and hence newly created instances of this component to potentially use a different field type than the pre-existing component instances?
- SDC supports default
*.component.yml
-defined values. But that won't work for e.g. a default image. Is it okay for XB to layer on support for that by auto-discovering apropname.(gif|png|jpg|…)
file in the SDC's directory? 🤔 (Will add this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Needs work , to potentially add that to upstream SDC.) - Is it acceptable that the default value sourced from a component's
examples
cannot be overridden on a per-site basis?
I think the answer to the first question is "yes". (Note that every stored
StaticPropSource
is self-contained: the user will continue to be able to edit it, even if it's a completely different field type.)If the answer to the second and third questions is "yes", then we can in principle remove the
Component
config entity. But that would introduce a huge risk: it'd make exporting/sharing configuration (in the future: 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active , but now:config/optional/field.field.node.article.field_xb_demo.yml
, which has config dependencies on theComponent
config entities it uses) brittle.Therefore I think it'd be preferable to keep
Component
config entities under the hood — without showing any of that in the UI. I think that would also keep the door open for supporting per-site default values in the future, plus it will likely help with 🌱 [META] Support component types other than SDC Active .If you agree, then I'll create three follow-up issues:
- one that generates
Component
config entities for all discovered SDCs 1) upon installing XB, 2) upon installing a new module or theme - one to remove
\Drupal\experience_builder\Form\ComponentEditForm
after that auto-generation is happening - one for computing the appropriate field type + storage for
$ref
-defined prop shapes
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
- 🇩🇪Germany Marcus_Johansson
Thanks @valthebald - they are fixed together with some other phpcs complaints. I will merge it into 1.0.x-dev now.
-
Marcus_Johansson →
committed 8ddde811 on ratelimit-quota-exceptions
Issue #3459836 by Marcus_Johansson: Introduce AiQuotaException
-
Marcus_Johansson →
committed 8ddde811 on ratelimit-quota-exceptions
- 🇫🇷France andypost
it needs new profiling in a light of https://wiki.php.net/rfc/opt_in_dom_spec_compliance
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I started with a test that gathers all unique SDC prop schemas. Then I started writing logic to automatically suggest the sane core-provided field type choice.
In doing so, I started treading on the territory of #3456008 — and I believe that we now should not do that issue anymore, at least not in its original scope: 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed .
I am now thinking that not using a config entity type might actually be the only viable choice, because the config entity being proposed here only is viable if there is a good way to transform a given
SDC prop schema
into aPropShapeInput config entity ID
— because that's how we could achieve sane dependencies.The only reason that worked in the original proposal, is because HEAD does not yet support the
{type: string, enum: …}
use case, which is AFAICT widespread among SDCs. Once that is considered, the only way to generate a config entity ID is by hashing the enum.The issue title + summary need a drastic update. See the new test class, which has 3 test methods that build on top of each other. I'll continue this MR tomorrow and will update this issue as well as 🌱 [META] Early phase back-end work coordination Active .
Sorry for the delayed update here, had paused on it due to health reason, un assigning it for now.
- @wim-leers opened merge request.
- 🇳🇿New Zealand quietone New Zealand
Moving component to where the Coding Standards issues live.
- 🇳🇿New Zealand quietone New Zealand
This problem is detected with phpcs which is run before the test suites runs.
- 🇺🇸United States TravisCarden
A few more people have successfully tested the add-on. One person had a mysterious problem that was solved by updating his version of DDEV. If we identify an actual minimum version dependency, it can be explicitly declared in the add-on config, just like you would a Drupal module. Another person is having trouble with certificates in the container preventing Composer operations.
In the meantime, I've fixed an issue with the Cypress binary getting cached on the host machine and becoming unavailable after restarting DDEV. I also added a symlink in the project root to the module (deep down in
web/modules/contrib/experience_builder
) for convenient access.I'm continuing to work on Cypress UI integration.
- First commit to issue fork.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@lauriii is not yet convinced, but he's not available to discuss it further, so to A) get this moving, B) ground the conversation with @lauriii more, getting started on this nonetheless.
- 🇺🇸United States TravisCarden
A handful of people have tested the add-on, and it seems to be working well. There have been a few hiccups that seem to be mostly about individuals' DDEV installations (e.g., out of date versions), but the add-on itself is working.
I'm currently working on adding support for the Cypress App (UI). I've never used it myself, so I'm getting up to speed on it at the same time.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@syeda-farheen It's been a week now, but still zero commits on the issue fork. Can you give us an update? 🙏
- 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
The last comment here is the reason I searched for an existing issue. My vote for "warning" instead of "error".
- 🇧🇬Bulgaria valthebald Sofia
@Marcus_Johansson: I've added a few comments, none of them are show stoppers
- 🇬🇧United Kingdom jessebaker
It seems there are actually three different aspects to this issue. So, for clarity, I see the breakdown as follows:
- The backend failing to render the preview and thus the preview is just a white screen
- Centralising the handling of any other error states returned when fetching from the backend/api e.g. when fetching the list of components
- React client-side errors being thrown causing the entire app to not render
I'm not sure, but 1 and 2 may actually be able to be addressed by the same error handling.
3 should use as described in #4
-
Wim Leers →
committed 0202b5b5 on 0.x authored by
tedbow →
Issue #3462742 by tedbow, Wim Leers, bnjmnm: CI: use a snapshot of core'...
-
Wim Leers →
committed 0202b5b5 on 0.x authored by
tedbow →
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
the new
phpcs-rules-match-drupal-11
job👏 Great call! 😄
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I don't — I have zero experience with Open API 😅
But a quick search in @larowlan's
decoupled_lb_api
module reveals https://git.drupalcode.org/project/decoupled_lb_api/-/blob/1.x/tests/src..., whoseassertDataCompliesWithApiSpecification()
achieves exactly that :) - 🇳🇿New Zealand quietone New Zealand
I did a light review of my previous comments and the comments in the MR. (It is noisy here and it is hard to think). There is just the one sentence we are working to agree on. I have made a new suggestion.
This time I noticed that that new classes are not declaring stricts types, Is there a reason? And there are new methods which are not defining return types. Should they? Oh, that noise is helping me to make mistakes. I didn't see your comment and the use of 'app root' in comments and I applied my suggestion. Sorry, about that.
Regarding ProxyBuilder, \Drupal\Component\ProxyBuilder\ProxyBuilder includes embedded strings that are placed in the generated files. I think it is much simpler, avoids another class and method, and with the bonus it easier to read (at least for me). As for the other I hadn't considered ManageGitIgnore. That all can wait for an opinion from another committer.
- 🇺🇸United States steyep
I could not get the patch in #74 nor the diff from the MR to apply cleanly in 10.3. I've attached a re-rolled the patch for 10.3
- 🇬🇧United Kingdom catch
The title no longer matches the issue summary and MR, can it be updated? Also one comment on the MR.
- 🇺🇸United States tedbow Ithaca, NY, USA
Think this is ready for review. I used the core/php.xml.dist file before 📌 Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard Fixed was committed.
You can see the shows a new warning that our
core.phpcs.xml.dist
file is out of date but phpcs job passes. https://git.drupalcode.org/project/experience_builder/-/pipelines/231385I also ran pipeline with were I copied the most recent core/php.xml.dist. This passed the new job to check to see if the phpcs was up to date but failed the phcs job because we are not up to date with all the rules. https://git.drupalcode.org/project/experience_builder/-/pipelines/231381
- 🇺🇸United States pfrilling Minster, OH
This has been resolved and the API Spec test should be passing now.
> # 1 The new test is failing:Do you have any examples for testing the fixtures against the spec? If not, I'll start researching how to make that happen.
- 🇺🇸United States tedbow Ithaca, NY, USA
I specifically opted to chase it because otherwise we get the feedback/complaint that we’re trailing HEAD too far.
Are we going to be in core anytime soon? I am unsure why it matters if we are month or 2 behind. does this really change that much?
Since I doubt anything will get into core before the demo in Barcelona it seems that we should favor development speed over keeping up with core's phpcs rules.
then? That'd match Drupal 10.x's rules, which won't change as much.
That seems better but 📌 Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard Fixed was still committed to 10.x branches.
I am going to change the MR to a draft try a couple of ideas. Basically allow us to get a warning if we haven't updated in phpcs rules in X number of days
- 🇩🇪Germany Marcus_Johansson
Ah, missed the response here. Right - the core would handle those in a unified with the change I made and according to how you write in the last comment.
The thing that is "missing" from your initial description is that it doesn't take care of flagging/disabling an provider. This would have to go into some 3rd party contrib module or a submodule to the AI module.
Pull request here: https://git.drupalcode.org/project/ai/-/merge_requests/27?resolved_confl...
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
(Too bad that core doesn’t batch these — that’d have made it much more bearable.)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I specifically opted to chase it because otherwise we get the feedback/complaint that we’re trailing HEAD too far.
But I agree that 2 breaks in 1 week is too much.
The Needs Review Queue Bot → tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- 🇳🇿New Zealand quietone New Zealand
Made an MR from the latest patch. The text reads well to me. I am not sure what @joachim saw, but the example after this text has used both the --sqlite and --dburl parameters since 2014.
- @quietone opened merge request.
- First commit to issue fork.