Added a small check, but there is probably a better way to do this. I notice that there is the $is_content_empty variable that was already being checked, but that does not seem to have been sufficient to prevent the error I described in #62. Any comments regarding how to clean this up are welcome, but this latest commit does fix the fatal error for me.
Thank you for the detailed comments by the way. I'm sorry I haven't had the time to review them, but I hope to be able to at some point soon. For now, I am just reporting that my client reported a fatal error and I found that the logs brought me to the code affected by this issue fork. If you have a field with multiple values, choose to display only some of them in the layout, and then remove those values from the node edit form, a fatal error will prevent you from being able to manage the layout:
Error: Call to a member function count() on null in Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray->onBuildRender() (line 124 of /var/www/html/web/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php).
Will update the fork to handle this.
I went ahead and merged the changes, but will keep this in needs review for the moment in case there happen to be any further feedback
Hey thanks for this update. It looks like there are some good things in this patch, but it also looks like there are some reversions, the most blatant of which is the re-addition of the FacebookUserManager class. Also, I can appreciate that you would make a patch in this way for your own purposes, but this patch really addresses several issues. If you happen to have a cleaner version of the patch, or were able to identify which changes were made intentionally somehow, that could be helpful.
I think it will make sense to update the documentation under the graph api version field. Currently it says to copy from facebook, but you are correct that it is an integer field, so really just use the up/down arrows to select the api version that matches the API version of your facebook app
For future reference, it is here: https://packagist.org/packages/nickdnk/graph-sdk
Thanks for the patch!
megakeegman → made their first commit to this issue’s fork.
I can confirm that this patch fixes the problem. Thanks!
This patch looks to have resolved this error for me
I attempted updating past the 2.0.0-beta6 release, and found that some of the changes in the code are preventing me from running database updates. This is on Drupal 10.4.1
I can verify that it is not related to database hook 10000, as I could run that hook, experience failures on other update hooks, revert my give code to 2.0.0-beta6, and then run those same update hooks successfully.
Here are at least the first few lines of that error:
> [notice] Update started: views_post_update_pager_heading
> [error] ArgumentCountError: Too few arguments to function Drupal\Core\StringTranslation\TranslatableMarkup::__construct(), 0 passed and at least 1 expected in Drupal\Core\StringTranslation\TranslatableMarkup->__construct() (line 129 of /var/www/html/web/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php) #0 [internal function]: Drupal\Core\StringTranslation\TranslatableMarkup->__construct()
> ArgumentCountError: Too few arguments to function Drupal\Core\StringTranslation\TranslatableMarkup::__construct(), 0 passed and at least 1 expected in /var/www/html/web/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php on line 129 #0 [internal function]: Drupal\Core\StringTranslation\TranslatableMarkup->__construct()
> #1 /var/www/html/web/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php(144): ReflectionAttribute->newInstance()
> #2 /var/www/html/web/core/lib/Drupal/Core/Plugin/Discovery/AttributeDiscoveryWithAnnotations.php(101): Drupal\Component\Plugin\Discovery\AttributeClassDiscovery->parseClass('Drupal\\give\\Plu...', Object(SplFileInfo))
> #3 /var/www/html/web/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php(84): Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations->parseClass('Drupal\\give\\Plu...', Object(SplFileInfo))
Not sure how this is happening yet, but I assume it is related to changes in usage of t functions.
While update hook 10000 is not the cause of the above problem, I am not so sure about this pattern of setting not recurring to -1.
Would like to look into this some more, but need to look at another module at the moment.
megakeegman → created an issue.
Happened to look right as you posted. The patch looks good. Can confirm that the export and import of shortcuts works as expected. And no more Invalid URI errors.
I can confirm this issue. I wasn't paying much attention when I exported. I noticed that this was a problem when I tried to import and started getting errors everywhere I tried to go due to having an invalid uri somewhere in my menu. Removing the broken shortcut fixed the issue. Afterwards I looked at the export and it looked as described above.
My only issue with this at this point is what I said above in comment #9 ✨ Configurable email Active
..and related that we have the timeout token categorized as a 'user' token. These things could probably be improved, but I am not sure what would be best practice for these.
Okay I think it's in good shape now
Whoops also going to revert some of the stuff in the generate function, since we don't really need the token functionality there either.
I went ahead and updated MR 18 to be consistent with the core user module. This removes the one-time-login-link token, and makes the one-time-login-url token accessible. It also moves all of the token replacement logic into the HOOK_mail implementation. Final note that I removed the inclusion of the login url from the alert. I don't think we need to show it to the admin that clicked the send login link button.
Okay I see that the major difference is that we are handling token replacement in the controller, rather than in HOOK_mail implementation
Trying to dig into this, it looks like the uli generation for the url token occurs in user_mail hook, which is module specific. So the URL token currently ONLY works for emails that are provided by the user module. I think the only way around this is to duplicate some of the code from the user module
Can confirm that the URL token does work when the patch is removed entirely, so we are breaking it somewhere.
Just tested and the login link is not actually making its way into the email via the token. Also, I see that a new token was created for this—one-time-login-link. I wonder if this can be updated to just use the existing one-time-login-url. But need to look into how the login link actually makes it into the token for that one. Because currently our code is somehow breaking the one-time-login-url token, so need to figure out why.
The new branch and MR is simply a fresh rebase onto 1.0.x
My UX questions / concerns still stand, but if you all are happy with the way it is done, I don't want my opinions on this to get in the way of getting this feature in, especially since the code is functioning and the feature does work as intended.
Your changes do appear to function though.
Looking back at earlier comments, I understand why this configuration was placed in the people admin area, given that it looks like the earlier ambition was to be able to change the valid time per user.
Looking at this, I see a few things that are a bit out of place. The configuration is under the people admin menu and pages rather than under configuration -> people. There is also a typo in the link /admin/people/configutre-password-timeout
I don't mind making these changes if a maintainer gives a thumbs up on tweak to the menu and routes. I am looking to get this issue completed in combination with https://www.drupal.org/project/one_time_login_link_admin/issues/3495380 ✨ Configurable email Active and it doesn't matter to me which one comes in first. Happy to fix any merge conflicts either way.
I forgot to change status to needs work, but status is accurately needs review again
Okay ready for review again. If possible, would like to get this in, and then update https://www.drupal.org/project/one_time_login_link_admin/issues/3379311 ✨ Allow option to edit valid time Needs review on top
I am actually taking a go at making one more commit on your branch. Shouldn't be long before it is pushed
I think it would make more sense for the email settings to be in the mail settings at /admin/config/people/accounts rather than on their own settings page. I had this part done on the original branch of the issue fork you want to use that for reference.
Hey, thanks for taking a look. The str_replace method you are using looks like it would work, but it is a little strange, since email configuration generally has good support for tokens. Do you think we should not add the token module → as a dependency?
megakeegman → created an issue.
Updated tests and fixed some of the linting errors, except for the eslint ones which I don't think are relevant to the changes here.
If I add a configuration option to do this, what are the chances of a merge?
New patch to apply after changes from 1.1
New patch for 8.x-1.1
megakeegman → created an issue. See original summary → .
mlncn → credited megakeegman → .
I am testing out MR 9 against 10.3 and unable to make it so that someone of a certain role can only get access to the secondary toolbar, and not the primary one.
I tried with and without the MR in the associated gin issue: https://www.drupal.org/project/gin/issues/3217391 ✨ Add permission for showing only the secondary toolbar Closed: won't fix
I did make sure that my role was given the access toolbar permission, as well as the access secondary permission, but they were still unable to see the secondary menu. I am only able to get them to see it when they also have the access primary permission.
Great, I am just about to push a commit to the issue fork
megakeegman → created an issue.
I think I am also seeing this issue, and I am very confused about how to go about troubleshooting. Any advice?
I tried that too :(
I thought the same thing and unfortunately I get the same problem. It feels like composer is just looking at the wrong thing for some reason. When I have a minute, I can try this out on a fresh install and see if I get the same thing.
Right, but I am constraining to 1.0.x still, not 1.1.x. So it doesn't make sense to me that composer is unhappy, since it should be cool with Drupal 8, 9, or 10, and I am running into this error while my Drupal Core constraint is set to 9.5.11
I did not recognize that there was a difference between MailManagerInterface and MailInterface: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Mail%21Ma...
megakeegman → created an issue.
I experienced the same issue and the issue fork resolved it for me.
megakeegman → made their first commit to this issue’s fork.
megakeegman → created an issue.
A small module for hiding the "Save and select" button: https://www.drupal.org/project/simplify_media_ui →
Just went through this with @mlncn
If you go to the media library settings at /admin/config/media/media-library
then you can enable the advanced UI. Enabling this adds a button to your media selection interface that says "Save and insert"
Otherwise, just make sure the required permission on your media library views is set to "Access media overview" instead of the default "View media"
More of an aside, but one other gotcha is that your user has to have permission to add media of the types in the field.
Once all of this is set up, you can use the media library widget to add media without having access to view the media library.
While this is great, it is all still very confusing to set up, and even still it would be great if the advanced UI was more configurable so that, for example, I could hide the "Save and select button" that doesn't do anything when your site is configured like this.
megakeegman → created an issue.
But I acknowledge that this might be a feature for some people.
I think this would also solve a lot of the usability challenges on this module, for what it is worth
Okay one thing I really don't like about this implementation is that when you change the order of the deltas in the edit form, including when you delete a value, the items that you have displayed in the layout change.
Oops I didn't realize that the changes I made affected things outside of preview mode. One more fix to make sure my index strings only show up in preview mode.
Sorry #53 was still buggy, so ignore that one. This new one is what I meant for #53 to be. I did not make any improvements mentioned in #54. Again, this patch is for 10.3.x, but if the changes are agreeable, happy to commit them into the fork.
Potential improvements to make on #53:
- make indices always display, even when offset and items to display are both 0
- Add block configuration to control whether the indices should be shown in the preview
I went ahead and made a small change to patch #44 (I made against 10.3.x), which addresses both the bug and my need for an index to be displayed. I am happy to take any feedback, and if people like I can update the issue fork too. I pretty much just touched the code mentioned in #51.
I can confirm that bug. Also I wanted to mention that this particular solution will be a bit difficult to use, especially in the case that there are a lot of deltas in field. I wonder if it makes sense to also display the index of each delta in the preview to make it easier to select the values you are looking for
megakeegman → created an issue.
megakeegman → created an issue.
Thanks very much for making this happen. Very useful feature, and it looks like it is working.
liberatr → credited megakeegman → .
liberatr → credited megakeegman → .
megakeegman → created an issue.
This would be super helpful. I would like this capability for taxonomy terms, but I'm sure it would be great on many other entity types.
Hi, Ed. I apologize that this is so difficult. I have done this upgrade a handful of times, and I know that it is challenging, which is part of the reason we have not been able to streamline it. I've got a deadline approaching next week, but I would be happy to meet with you the week of the 21st. Feel free to message me vi drupal.org or slack to coordinate
For anyone wondering, patch #283 and #284 are the same, #284 is just cleaned up with documented return types etc...
Re-tested the migrate -> rollback -> migrate
and it looks like errors are gone and things look as they should in the database
megakeegman → created an issue.
I tested the fix and am now able to see that I can save non-ascii characters into the value_hash column
megakeegman → created an issue.
Ohhh I just realized that the link is under the "search and metadata" menu
megakeegman → created an issue.
I did not mention in the original comment, but I was considering that we would not need a permission for create, given that I think someone who is allowed to edit a tracked field's parent entity should be allowed to create a value revision for that field.
megakeegman → created an issue.
Updating the patch. Patch 2 was missing the parens on a function call. Patch 3 adds a revision timestamp and user id.
Here is a basic patch that does what I need it to. Very happy if anyone has improvements or recommendations for improvements.
megakeegman → created an issue.
megakeegman → created an issue.
Worked with @lelkneralfaro on this one, and we were able to sort out that creating the fields in the way I describe above does not do anything. Rather, what was missing, was that we had to
- go to /admin/config/search/value-revisions
- select the "content" check box
- select the field name for our regular term reference field
- and finally save
I was confused initially because the first time I tried this, I selected "Taxonomy term" instead of content, but what I did not understand is that selecting "Taxonomy term" at the top opens up the list of fields that are ON taxonomy terms, and not the term reference fields themselves.
Updating this issue to be about improving documentation in the UI.