Re-roll of the MR fix against 10.3.x.
I have re-rolled the patch from 79 against 10.3.x.
Reroll of #34 against 10.3.x
Love this patch. I have a slightly simpler version here that checks to make sure the parameter is marked as "entity" in the route.
The tab appearance for non-webform objects has been fixed elsewhere and is in 5.1.
Definitely seems appropriate, and the code looks good to me. Let's call it reviewed but not yet tested, once we get some testing in this should definitely get merged into a release.
The use case is anonymous orders. This is especially relevant for subscriptions that grant special access. Someone buys a recurring membership anonymously, and then the order gets reassigned to a real user (which I believe is part of a standard commerce feature). Perhaps this approach is too general, and we should just be responding to reassignment of anonymous orders.
I don't know that I am confident in that assertion -- been a while since I had my head in this code. Certainly it would be nice if this fix weren't necessary -- it's a little clunky.
Re-roll of patch in 17 with D10 compatibility fix.
Here's a patch for convenience.
It seems like something went wrong with your initial setup -- if you can provide some reproduction steps I'm happy to have a look but I don't get a reproduction. Sorry about the documentation, it certainly needs some attention, and you correctly identified that relation in D7 became Connection in D8+.
@ankithashetty I haven't experienced that bug, but it sounds like it merits its own ticket. While it is related to the same feature, it is not related to the same bug: this ticket was just for the form not being able to list the record types anymore. As far as I can tell, nothing about this code has caused any new bugs.
Please provide more information in that ticket if you can, as I'm not able to get a reproduction. Can you tell us what the "conditions" property on the Soql query are when it is dispatched to Salesforce in `\Drupal\salesforce_pull\QueueHandler::getUpdatedRecordsForMapping` for example? And share the mapping configuration yml file? Adding those to your new issue will help us reproduce the bug.
gcb → created an issue.
Here's the current state of the MR as a patch file for stable use in composer builds. This applies for me on 10.2.4.
It happened for me on PHP 8.1! I was suggesting this solution is better than changing the "isset()" check to something else, actually, and that this was indeed a good long-term solution because it makes our object behave as intuitively expected, rather than simply avoiding the confusing behavior and leaving the trap in place. It's possible we need to implement this function in other objects as well, though.
We can fix by getting more clever with our check, or by making the Rest object generally respond better to isset checks, which seems like a better general solution.
Re-rolled against 5.0.4
Another re-roll against 5.0.4
Patch version that applies to 5.0.4. For use in composer patching so we aren't using an unpredictable reference to an MR, until this gets merged into a release.
Since there hasn't been a "next release", this issue is still relevant. I am posting an updated patch that applies to the 3.0-alpha1 release, and will post one for alpha2 shortly, unless the commit works on its own as a patch.
I have the same issue, manifesting when I load up the layout builder configuration for a view mode for the entity.
This was actually caused by a patch on issue #2871483, so I have fixed that patch instead.
I found that the patch above caused a crash on free orders that didn't have a payment method attached when trying to produce the payment information summary pane. The attached version makes a small check to avoid that crash.
Thanks for the feedback @mariacha1. How about now? I went ahead and added a schema file for the Contact settings as well.
This functionality has been added separately.
Merged and will be in the next release (or RC)
Confirmed this is still a bug. Here's a stack trace of where the exception occurs when we make a call to \Drupal\salesforce\Rest\RestClient::objectDescribe()
Promise.php:79, GuzzleHttp\Promise\Promise->wait()
Client.php:189, GuzzleHttp\Client->request()
HttpClientWrapper.php:40, Drupal\salesforce\Client\HttpClientWrapper->retrieveResponse()
SalesforceJWTPlugin.php:177, Drupal\salesforce_jwt\Plugin\SalesforceAuthProvider\SalesforceJWTPlugin->requestAccessToken()
SalesforceJWTPlugin.php:188, Drupal\salesforce_jwt\Plugin\SalesforceAuthProvider\SalesforceJWTPlugin->refreshAccessToken()
SalesforceAuthProviderPluginManager.php:189, Drupal\salesforce\SalesforceAuthProviderPluginManager->refreshToken()
RestClient.php:190, Drupal\salesforce\Rest\RestClient->isInit()
RestClient.php:200, Drupal\salesforce\Rest\RestClient->apiCall()
RestClient.php:512, Drupal\salesforce\Rest\RestClient->objectDescribe()
There's a call there:
throw Create::exceptionFor($this->result);
It's throwing an exception! And no one catches it. Walking it back up:
GuzzleHttp\Client->request() states "@throws GuzzleException"
That bring us into salesforce module code:
Drupal\salesforce\Client\HttpClientWrapper->retrieveResponse()
I think the trouble begins here, as this states:* @throws TokenResponseException -- I don't think that includes a GuzzleException, so we probably need another "throws" here. Note that this comment is via @inheritdoc. Anyhow, the code doesn't acknowledge/handle the exception otherwise.
SalesforceJWTPlugin->requestAccessToken() also says it throws the token exception while ignoring it.
After that, inherited comments start to acknowledge a MissingRefreshTokenException, but again, not handling them.
Based on the names and purposes of those functions, I do think it's reasonable for them to just throw exceptions -- they can't do the thing they want to do, which is to get a token!
This takes us back to `->isInit()`, which presumes to return a boolean telling you whether or not the RestClient is initialized. In this case, I think we can confidently say "it is not" and we know that because an attempt to refresh the access token causes an Exception to be thrown. We should catch this exception, log it, and return "FALSE".
TLDR: This is still an issue and @dspachos patch looks good to me. I'm not entirely sure why the change to `SalesforceJWTPlugin` is needed, but it seems like a clean and happy piece of code.
Huh. The last patch doesn't apply in builds because it patches the info file and the info file has extra junk in it from automated D.O. packaging. Here's one usable for those cases.
Attaching Snapshot PR of the latest version of the Merge Request by mariacha. works with the latest release and tested on D10.
Here's a patch version of the PR as of comment 74, along with patches for commerce_stripe and commerce_authnet to make them compatible.
I suspect those probably belong in the other modules' issue queues, but until we are sure this is how things are being done I think it makes sense to keep it all in one place.
Updated patch & PR to prevent overwriting of a user's existing customer ID. This commit seems relevant beyond this issue's specific problems, as it's certainly possible for a user to have a customer ID, then make an anonymous purchase using their email address, which will then break their existing saved cards without a fix like this.
Discovered an issue with the latest solution, which is that a payment intent can't use a payment with a different customer ID. You can see this issue emerge if you select saved payment method associated with a different customer ID (one using the field added in this patch), then go back to change payment to a different or new payment method.
This is feeling like a generally bad approach to solving this problem. Instead, we should probably be smart about associating the new user with the customer record in Stripe.
However, here's a workaround which refreshes the payment intent.
Reroll diff.
And another reroll that lets the field configuration page load.
And how about a reroll that doesn't cause whitescreens!
Patch reroll against 1.3.
Re-roll most recent patch against dev latest (a2c4813d)
I'm confused at why this is "won't fix". Allowing the email trigger transition to be configurable seems very reasonable. @jsacksick you say that the patch references "state" where it should reference "transition" and you are right, of course, but are you saying that's the reason you aren't going to accept the patch? Because someone labeled their variables incorrectly? I can fix that if it will maybe change your mind! The functionality is still good IMO.
I thought Commerce Recurring did use a "place" transition. For whatever reason, it just does that before "paying", and so maybe that should get fixed, but until it does, this would be a really helpful workaround that only makes Commerce more feature-ful. I see you listed as a maintainer on Commerce Recurring, so perhaps you can clarify for us what we are doing wrong with some more actionable feedback?
I'm attaching a patch that changes only 2 things from the one in 27:
1. It changes the configuration option label to "transition" rather than "state". (I didn't rename all the variables etc, but if the maintainers would consider committing if I did, I can do that.)
2. It changes the name of the update hook to be descriptive rather than numeric so you won't have collisions as time goes on. I can't figure why commerce is using numeric post update hook names -- that is a requirement of hook_update_N but not hook_post_update, and that was done on purpose. I'm assuming in part to avoid problems like this! Anyway, the numeric restriction is probably just going to make messes in contributed space like this -- if I use the patch above, I'm likely to miss a future commerce update hook and end up in big trouble.
@Strakkie Thanks for the patch!
The warning fix is definitely needed. However, the other part breaks the module. The way this module works is that you create items, and then you add content to them. That's the content. Note the function "content_snippets_config()".
Items include a name, a machine name, a type, and a description for your snippet. Those appear under the "Content -> Snippets" for site editors with the proper permissions to set values for those. So, you need to create the item and THEN set the value for it as separate tasks. The idea is that the sitebuilder or developer creates the items, and leaves it to the content editor to be able to change the text. This help make site content editable rather than hard-coded for custom usages.
I've submitted an MR with a solution to this issue, which makes sure customer ID's are associated with payment methods that we are saving, then retains them and assigns them to the User that gets created by Commerce.
I'm attaching a patch of the current MR for convenience.
@Berdir the core module layout_builder regularly renders content with sample entities in its UX. These sample entities do not have IDs. As a result, if you use a URI token anywhere on a default layout, the layout builder screen is crashed by this issue.
The function \Drupal\Core\Entity\EntityBase::toUrl() begins with a check for a Null entity ID, and throws an exception if there's no ID. Passing an entity to this function without checking for an ID first is asking for whitescreens -- new, unsaved entities are valid for rendering according to Drupal standards. See the discussion in
https://www.drupal.org/project/drupal/issues/3069001
🐛
Layout builder's use of sample data can cause exceptions during rendering of default entity view displays
Needs work
about having contrib modules sensibly handling "preview" content.
I have to agree with @Anybody here. While it certainly sounds reasonable to ask all contrib modules to respond coherently in preview mode, it's not realistic: this is the perfect being the enemy of the good. If Drupal's standards for publishing modules were different, or if the feature sets offered by fully mature modules were more complete, this would be fine. It's not, and this issue has cost our clients a LOT of money, as well as frustration.
How hard would it be to catch exceptions and put something in the preview like, "preview failed for this field" below the field name in those cases? This would highlight contrib fields that needed attention without showing a whitescreen to users. It would also make it easier for developers to debug!
I have this whitescreen right now and it appears to be coming from a combination of entity references and the use of Tokens. Token is a pretty well-supported module... but there's a bug reported for it in the queue 🐛 Error: "The 'node' entity cannot have a URI as it does not have an ID" when using [node:url:relative] token Postponed: needs info that points back to this issue, and the maintainer basically saying "sounds like a layout builder problem".
As a site developer, my options here are to stop using token, to hunt down the bug in token and submit a patch (which the module developer is convinced they don't need, so I may have to maintain it indefinitely), or to apply this funky patch to layout builder that sort of blows up the entire preview behavior anyway.
I've been through this with a dozen different modules at this point. As a result, my org is very unlikely to ever use layout builder on a new site again -- it just wastes too much time hunting these things down. I like layout builder, and I want to use it! Can't we just be a little bit more accepting of our imperfections and put some safety measures in place?
gcb → created an issue.
One more update here to also check all the cc and bcc addresses. The result is that the whitelist only allow send if ALL recipients are whitelisted, which may suggest adding some more clarifying language on usage.
Yes, as I said, it's on the dev branch. There hasn't been a release since it was merged, so you have to use the dev branch or treat the commit as a patch if you want it.
Hrm, please ignore the extra branches an noise I made here. The fix is already in for the form/edit mismatch on the 8.x-1.x branch https://git.drupalcode.org/project/field_permissions/-/commit/003b4f069a...
1.1 broke these patches. Here's a try at updating the full refactor one for 1.1. I will also turn this into an issue fork.
Finally got back to this and cleaned up the form UX and other review feedback, I believe. Please let me know if other changes are needed.
Parallel issue: Contacts don't exist when a user registration email is sent, meaning tokens aren't available yet. Attached patch fixes both these issues with a refactor to how we handle registration.
This probably needs some more work to line up all the settings. We currently have a setting "Link to existing contacts" that essentially does nothing. This patch acts as if it is always turned on when user/contact connection is active -- so the setting is still useless.
However, this patch is necessary for the basic version of "create contact on registration" now, so we can't simply base this behavior on the Link to Existing setting.
Fixed in dev, commits didn't tag this issue. Will be in 2.1.0.
This feature is very desirable. I've updated the patch to be a little more robust and apply against the latest release.
Re-rolled patch for 5.0.3
This seems very sensible!
The ability to work around an issue seems like a strange reason to say it's not a bug. The legend is intended to be visible as part of the layout paragraphs UX, is it not? If that's the case, why would the module hide it by default?
Ok, labeling improved, and also a fix for the form behavior. Tests aren't here yet but let me know what you think of this UX language!
It seems to me like there's a fundamental contradiction to running database updates and configuration updates all in a single action.
It's already very complicated to wrestle with the concept of "correct configuration" when you have site administrators making configuration updates and developers making configuration updates in parallel.
What if configuration updates in code were given special treatment? We could have a separate one-time operation from updb [hook_install_configuration_N(): returns an array of configuration files to be re-imported from a module's /config/*/ directories; is triggered by 'drush update-configuration' or visiting /update-config.php]
Then, the "best practice" could be:
1. updb
2. cim
3. upcfg
(and then: export that config into code!)
If we tracked the executed upcfg functions in their own table with a date or "invocation" column, we could provide a way to "re-run" an invocation if a site accidentally ran CIM after upcfg.
Sounds good to me, and yes this patch works great for my use case. Thanks!
Yes -- it's what I expected. The entity is new, and doesn't have an ID. $entity->id() returns NULL. $entity->isNew() returns TRUE.
I get this all the time with layout builder and various contrib modules that aren't expecting to generate sample content for an entity generated only for the purpose of LB's preview. (If you are trying to reproduce, but editing the layout on a specific entity, you might not be able to -- I think it uses the actual entity in those cases.) This is the layout configuration at the bundle level.
There's just no entity ID to work with.
I have Commerce products that are used to purchase event registrations. Basically, the Product has an entity reference to an Event node. We want to display some information about the Product on the event automatically. In this case, it's basically a field on the Product for the maximum number of registrations allowed.
The Product has a view mode that displays this integer field, called "Capacity".
The Event has a computed field using the reverse entity reference plugin, pointing to the product.
Events have a view mode, NOT using layout builder, that includes this and some other fields, called "key_details". It renders the computed field using a the Capacity view mode.
The Event "Full" view mode uses the ctools "Entity view (Content)" block to render the event using the key_details view mode. (Yes, the event view mode renders the same event in a different view mode)"
I suspect that extra layer of abstraction is unnecessary, and you could get the same thing with the computed field in a block on the Full view mode -- but that field still isn't appearing for me.
Here's the error, which I get on my event node type's full display mode configuration's layout config page: ("/admin/structure/types/manage/event/display/full/layout")
TypeError: Drupal\computed_field\ComputedFieldBuilder::viewField(): Argument #2 ($entity_id) must be of type int, null given in Drupal\computed_field\ComputedFieldBuilder->viewField() (line 50 of modules/contrib/computed_field/src/ComputedFieldBuilder.php).
Drupal\computed_field\ComputedFieldBuilder->viewField('node', NULL, 'computed_registration_product', 'key_details')
call_user_func_array(Array, Array) (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #lazy_builder callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 788)
Drupal\Core\Render\Renderer->doCallback('#lazy_builder', Array, Array) (Line: 353)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 204)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 160)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 161)
Drupal\Core\Render\Renderer->renderPlain(Array) (Line: 175)
Drupal\Core\Render\Renderer->renderPlaceholder('', Array) (Line: 665)
Drupal\Core\Render\Renderer->replacePlaceholders(Array) (Line: 550)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 204)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 148)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 149)
Drupal\Core\Render\Renderer->renderRoot(Array) (Line: 278)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->renderPlaceholders(Object) (Line: 127)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAttachments(Object) (Line: 45)
Drupal\Core\EventSubscriber\HtmlResponseSubscriber->onRespond(Object, 'kernel.response', Object)
call_user_func(Array, Object, 'kernel.response', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.response') (Line: 202)
Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object, Object, 1) (Line: 190)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 718)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Ah hell, that's bad reporting on my part. I was using it in a display mode that was being used as a rendered entity.
Maybe I'll make it up to you by figuring out how to get computed fields to show up in the field list in layout builder ;)
Thanks for the persistence and prompt review/help!