I like this, maybe we just need to modify project browser to allow displaying some examples of how they look when a user chooses them.
@pdureau I agree with what you're saying, I think SDC is just a part of the puzzle for customizing bits and pieces.
E.g. adding an accordion and being able to swap out the front end piece for a different SDC would be great.
Thank you for reviewing!
I have two follow up questions:
1. I thought the expected behavior in this case is to open the issue in the respective queue and leave a comment that those are unfixable rather than no issue at all.
2. Both of these deprecations are marked as not being removed until Drupal 12, they should not impact Drupal 11 readiness, what causes them to be unfixable if rector doesn't need to change anything?
Thanks!
Ah I see now, that then makes more sense, if you're restricting it to strictly Starshot / curated recipes then running them during the install step can be more accurately tested and that would probably be a better experience overall.
I also don't know the value in putting it during install, since I think a lot of infrastructure needs to be installed to pick it up, I wonder if it's technically an interstitial between install and visiting the site.
I think it's a small distinction but important.
I think it may be out of scope for this issue, but one of the issues is also that views config updates often require fixture updates which is a whole layer of complexity since they are zipped.
Why point number 2 to restrict to local recipes only?
@dww mentioned on slack one area for possible improvement:
Yeah, it could potentially be refactored to not make it so hard to use. Eg this badness of the class trying to save changes that then trigger views_view_presave() instantiating the class again to redo the work, etc.
It's what godotislate said in slack, I assume it means Out Of Scope.
Are there any blockers for this? I'd love to see a full release of this.
Thanks guys. As far as the profile question it is resolved, I was on my phone so I couldn't see the changes super accurately.
collectModuleHookImplementations takes care of it I think so I've removed that.
I also want to document the .theme discussion from slack here a bit clearer.
From godotislate:
you can implement hook_form_alter, hook_page_attachments_alter, among a few others in .theme
but that is via theme manager, not module handler
it looks like they are all invoked after module handler has its turn
so keeping them OOS seems fine
Can you please add those changes to the merge request? People can then generate local patches as needed and the core team uses the actual mr for testing and merging.
I'm wondering if the merge request being in draft is intentional?
My understanding is that this is ready for a deeper review with the following comments that all need to be addressed.
1. A framework manager needs to review per 61.
2. The suggestion to change FormAlter(FormTestAlterForm::FORMID) to FormAlter(FormTestAlterForm::FORM_ID) per 63.
3. One question in the MR about handling the profile extension that I think may be addressed.
4. The question in the issue summary about whether additional tests are needed.
Let me know if I missed anything, I added these to the issue summary too.
I addressed your comments, but I have one question about replacing configuration, I'd like to rename them to be more informative and accurate.
I've noticed some other modules that don't seem to have the auto issue created either:
Forgot to mention I also reviewed the change record which looks good too!
This looks good to go, I updated the summary to be clearer.
Tests are passing and the video confirms it is working as expected as well.
The feedback on the MR has been addressed as well so I think it is ready.
Looks good now!
This looks good, but I'm not sure the test is actually working.
For some reason I can't run the test only job, but I pulled the branch down to run it without the return and the tests still pass.
It took a bit of effort due to the phpunit 10 update, but I needed to handle that eventually.
If someone that has permission can run the test only job that might confirm my local findings.
Please let me know if this is the right direction.
I think this is missing the stub, I am not sure what the process is so I did a quick stub that just implements cachebackendinterface.
If you can provide some insight on whether this is the right direction I can work on it more, but I'd like some confirmation this is the right direction.
Re reading this is unrelated to this module.
Actually removing the submodules is more complex than I originally thought.
I still am not a fan of the auto install bit, I think though it might make sense to post a banner somewhere if modules such as geolocation are installed that mention you should install the submodule for functionality.
For now I am going to update the readme and remove the rest_views_modules_installed hook so you can test.
I'm looking to tackle this again. I think what I will do is the following.
1. Move the sub module functionality to the main module and wrap them in a check for the classes so they will automatically become available.
2. Make the current submodules stubs
3. Auto uninstall them on an install hook
4. Drop them in a 4.0 if we ever move to it
Does this seem reasonable?
Ok that worked!
A couple of notes, none of these are critical, but observations.
1. When checking store full request the request is escaped:
"content":"[\n {\n \"organization_name\": \"ABC Corp\",
I'm not sure I would even change this.
2. When store full request is checked you don't also get the response, though it's embedded so I don't think that should change either.
3. The Store full request option is not preserved on subsequent edits. Steps to reproduce below.
1. Edit json field and enable ai interpolator
2. Check Store full request under advanced settings
3. Save
4. Edit same field
5. Open advanced settings
6. Store full request is not checked
The typehint is: \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher $eventDispatcher
But you're passing in just '@event_dispatcher'
and the use statement is Symfony\Component\EventDispatcher\EventDispatcher
I think you just need to align those, I'm not sure which one you wanted.
Ok I was able to get the dev version for both.
I now see ai interpolator for json AND I see the option to dump the full value.
When I try to save content I get the following error:
TypeError: Drupal\ai_interpolator\AiInterpolatorRuleRunner::__construct(): Argument #3 ($eventDispatcher) must be of type Symfony\Component\EventDispatcher\EventDispatcher, Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher given, called in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\ai_interpolator\AiInterpolatorRuleRunner->__construct() (line 42 of modules/contrib/ai_interpolator/src/AiInterpolatorRuleRunner.php).
@joachim, thanks for your insight, I think that would be certainly helpful, and I think bbrala has been experimenting with core rewrites.
I think it's slightly off topic for this thread though as this thread is more about providing support to the team behind rector and phpstan so they can provide insight and be notified of changes that require rector / phpstan updates before they land.
The current process requires a lot of effort on their part to remain informed and they are always chasing. Ideally we can be more proactive to help with such important tools for keeping contrib up to date.
There is something strange going on with the project, I think you have to enable dev releases.
If you edit the main project page then scroll all of the way down and click on issues, then scroll all of the way down again you should see an option for Version options.
Are both the tagged releases and development version checked (see screenshot)
I don't see a dev version on the project page and even though I included dev it is only grabbing the rc1 still.
There are no json field rules.
If you check that dev version it should make it available.
Ok I downloaded both dev versions, I don't see any options on json fields yet.
Looks good to me too.
It seems that @quietone's concerns about the comments have been addressed as well as they both read clearer.
I'll take a look!
I think for the store changes we either provide an update hook, or just post a notice.
Thanks for looking here are my thoughts.
1. I think long term this is great for additional integration and flexibility.
2. I think this would solve my current issue since I am trying to post process the changes so I can create multiple entities based on the response.
3. I would just make the option dump it at the bottom of the long text field rather than specifying a second field, but a second field is acceptable too.
Yeah I think making it configurable would be great!
For the record I did try prompting it to send it to me as a comma separated value before posting and is still put them in separate objects.
I tracked down the issue, in OpenAiStringLong.php
storeValues just passes the array to the entity set.
Ideally we could ether configure the expected response or we could implode the values, adding $values = implode(' ', $values); in there merges everything into one field.
Ah I included too much in the api url, I'll do an MR in a bit adding some documentation.
Thanks for the example!
I updated the descriptions.
I also changed the two tokens for commerce_store_* to commerce_tax_* since they are tax objects
and I split profile into two since one of them is an array of profiles.
Great! This shouldn't affect you then, I'll close this!
Is there a reason not to make them translatable? I had trouble finding documentation on attributes of Attributes.
Can you give me an example of other tokens you've set up? When I searched a couple I didn't see examples.
Sorry I missed that, I was creating a followup since those ignores were still in core and I didn't see it here.
I'll close this, thanks!
Sorry my follow up comment wasn't clear, you probably won't have an issue, but you are including core in your project which is not generally recommended.
https://git.drupalcode.org/search?search=country.default&nav_source=navb...
You can see the references in the link above, feel free to close this.
This is likely not an issue for Address: https://git.drupalcode.org/search?search=country.default&nav_source=navb...
Quick glance on how it's used: https://git.drupalcode.org/search?search=country.default&nav_source=navb...
Quick glance on how it's used: https://git.drupalcode.org/search?search=country.default&nav_source=navb...
You only read from it so you might be all set, but you might want to handle NULL.
https://git.drupalcode.org/search?search=country.default&nav_source=navb...
Looks like you only read from it, the second instance might need some checks:
https://git.drupalcode.org/search?search=country.default&nav_source=navb...
Looks like you are already setting a default so you might be good: https://git.drupalcode.org/search?search=country.default&nav_source=navb...
It looks like you are already setting a specific default, but you might want to read through the attached issues: https://git.drupalcode.org/search?search=country.default&nav_source=navb...
This seems like it is due to including core in the codebase which means it's probably not used directly.
nicxvan → created an issue.
nicxvan → created an issue.
nicxvan → created an issue.
nicxvan → created an issue.
nicxvan → created an issue.
nicxvan → created an issue.
Yeah, that's why I created these issue, thanks for reminding me to finish.
The way you use it is one of the reasons we removed the default country from the install page, core didn't use it at all and most contrib just used it like you are here to either set it themselves or use it as a default internally.
If you're not using that value in your system, it might be better to just remove it unless you have plans to use it in the future.
There is some discussion to add it back into the date formatter so that depending on country dates get output as your countries default.
So it might be worth considering. In the minimum I think what you mentioned is line 96 should be NULL instead of ''
Ah, of course, for some reason I thought we could just change the target of the issue.
We probably want to leave this open though for the 2.x version in case there are more deprecations.
Not sure what is going wrong with the test. There was a long discussion in slack about being able to delete the cart when it's combined no matter what so I added that here.
The test should be doing the following:
Log in as admin
Add item to cart
Store cart id for later comparison
Log out
Add item to cart as anonymous
Store anonymous cart id for later comparison
Go to checkout
Log in as admin
Confirm the current cart is the anonymous cart and it is combined.
This is how it works locally, but the comparison assert is failing.
Setting to needs review for advice.
Took a pass at the CR.
The logs only show what I have shown here, however, I think that the root cause is here: #3282729: Always use checkout cart as main → .
What is happening is that if a user has multiple carts and logs in during the checkout process many times the order that they are checking out is the one that gets combined into the other cart. This shuffle is causing several bugs one of which is this exception.
Other issues it is causing:
The user is on a checkout on a cart that technically no longer exists so they cannot continue to review.
Google tag throws exceptions for trying to remove items from a cart that no longer exists.
I'm happy to work on the approach you discussed if you can point me where to start.
Thanks! I also just want to make it clear, this probably wants to make it into a recommended release before Drupal 10.3 drops since that introduces the upstream change.
Thanks! I also just want to make it clear, this probably wants to make it into a recommended release before Drupal 10.3 drops since that introduces the upstream change.
Should we be using the patch in 27 as the base? I think it seems the suggestion was to throw an exception if invoking hooks in middleware?
Sorry about the premature rtbc. My understanding is you link to the CR, the CR has a link back to the issue if there are additional questions.
Also you can add helpful details to the CR.
nicxvan → created an issue.
nicxvan → created an issue.
nicxvan → created an issue.
It is used in the KernelTest and the .module
nicxvan → created an issue.