🇺🇸United States @MegaKeegMan

megakeegman → created an issue.

megakeegman → created an issue.

megakeegman → created an issue.

I was just checking out the format guidelines added here:  https://www.drupal.org/node/1588136/revisions/view/13329945/13329953 → (just a couple comments ago).

I noticed the 750 word maximum and was a little bit surprised, since that seems like a rather low character count for a case study. Should we increase that number, or just remove it maybe?

megakeegman → created an issue.

I'm also looking at this, but was looking on the 1.x branch. Regardless of branch, do you think it makes sense to just add https://www.drupal.org/project/fontawesome → as a dependency, rather than trying to manage the fontawesome library directly?

Adding the other error for completeness

I have the initial error handy, so adding it to the description to improve documentation. I had tried resolving the syntax errors I recall, which eventually led me to different errors about the DeprecationHelper class.

I have been able to test to some extent. As expected, flushing cache does not work without this patch, but it does work when the patch is applied. Also without the patch, I was unable to apply database updates, but the patch also fixes that. I have not experienced any errors related to this while in the Drupal UI (aside from flushing caches), or any other d9 compatibility related errors coming from this module.

There was only one merge conflict during the reversion, in phpstan. It was the line 17 addition of a code comment phpstan.neon (at least it was line 17 in https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136/di...). In later versions, that code was uncommented. I assumed that we wanted to keep that line uncommented, rather than removing it. I'm sure you will say something if this is wrong, but wanted to point it out.

Oh wow, it occurred to me that that's what was happening, but I just assumed that I didn't understand the code. Anyway, option 1 seems okay, but unfortunately it may not be easy for me to test (at least without spinning up a blank drupal instance). In my attempts to get this current site to the latest version of D9, I have seen so many errors from modules, mostly relating to code updates that are incompatible with php 7.4, hence my assumption that informed the title of this issue. While I have been able to work through the handful of errors that prevent me from updating composer deps and running database updates, I am not really interested right now in fixing all of the other fatal errors that are preventing site usage, since I don't intend to stay on D9. Anyway, I guess the most important takeaway of this message is that it is slowly becoming increasingly difficult to update a Drupal 9 site that has contrib modules installed. I suppose I could try cloning the repo (issue fork) into my project directly (pre-update) for testing purposes.

I was thinking for my purposes to just add a patch that removes that line, and then remove that patch once I complete the upgrade.

I am happy to follow your guidance on what you think is best for issue queue management here. I'll check in for your feedback later.

Actually I was just reviewing the issue, the code that threw the error (sorry I don't have the error, was dealing in bulk on Friday) is in the Toolbar Controller.

DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', fn() => $this->assetQueryString->reset(), fn() => _drupal_flush_css_js());

If I understand correctly, DeprecationHelper class was introduced in Drupal 10.2, correct? If that is the case, maybe support for Drupal 9 should actually be removed? And maybe this issue is broader than my initial description.

megakeegman → created an issue.

Small change. And notably, I haven't tested whether there are more errors upon actually using the site. I am only using this so that I can successfully update composer deps and run database updates.

megakeegman → created an issue.

I used this diff to help perform composer updates. I found there was another small fix that I needed in order to fix database updates. Maybe this is a different issue, but leaving an updated patch for now (specifically a patch that allowed me to get to d9 latest on php 7.4 and perform database updates)

When running db updates, my site didn't know how to parse the .../Drupal lines in the .install file, which is what my changes address

Okay, will make an issue fork at some point, but I found I was able to get the revert buttons to show up by removing the 'revert' and 'revert revision' items from the array on line 441 of wse.module in the wse_entity_access hook. For reference, this code was added in https://www.drupal.org/project/wse/issues/3490853 📌 Allow viewing and reverting to live revisions inside workspaces Active

Is this or a similar change something that would be considered for merge?

Thank you for the info. I'll take a look to confirm what you are saying later today, and then move this issue to the correct project. I had assumed that this was something that core prevented intentionally, and that this module should work around it. A reasonable assumption, but good to know that it is not true.

Here is an example of what I see when I am looking at revisions from the default workspace (no revert button)

One potential UX issue that may arise out of this change (which is a typical issue with workspaces) is just helpful contextual clues or maybe just more warnings when trying to revert, and a reminder about which workspace you are actually in and which workspace the reversion will belong to. Just a consideration, but probably not required for completion.

Okay. So the moment content gets added to a workspace, you can only revert to revisions of that content while in that workspace. If you edit it again on live, you cannot revert to a revision. If you edit in another workspace, you will not be able to revert while in that workspace either. There is no error. Simply the "revert" buttons are missing while view the revision history. They are only present in the first workspace where the content editing occurred.

Currently I have edited content in a workspace called "Stage". I am able to revert to previous revisions while I am in "Stage". But say I switch to live and made edits in live—I am not able to revert to previous revisions while I am in live. I can do a little bit more testing to try and understand more precisely how it behaves.

megakeegman → created an issue.

Also to be specific (though you can probably tell from the error), this error specifically occurs when the workspaces extra submodule, wse_menu, is enabled

For what it's worth, I just experienced the same fatal error and then did this exact fix, and am pleased to find someone already having gone through this.

megakeegman → made their first commit to this issue’s fork.

If desiring to keep iframes out of media, https://www.drupal.org/project/ckeditor_iframe → provides a great way (in ckeditor5) to add iframes and edit their attribute values, and all without requiring a migration to media.

Just wanted to check in if there is anything that might be blocking this issue from being merged, or if there is anything I can do to help move things forward. This issue is a bit of a blocker on being able to authenticate to facebook with social post facebook. Thanks all.

Patch is not updated, but fixes were added to the MR to account for changes to the NetworkBase constructor in social_api

This issue caused a site to stop posting to Facebook. As of the latest commits on the MR, the site was able to post to facebook again. These changes address a few of the changes that occurred in social_api:
- dependency injection for network manager: https://git.drupalcode.org/project/social_api/-/commit/47ffc2c97683f0c19...
- also noteworthy that we are waiting on an upstream fix from social_api that may also cause issues with authenticating: https://www.drupal.org/project/social_api/issues/3501411 🐛 Error: Typed property Drupal\social_api\SocialApiDataHandler::$sessionPrefix must not be accessed before initialization in Drupal\social_api\SocialApiDataHandler->getSessionPrefix() Active

So anyone following this issue, be sure to check whether this issue has been resolved or if you still need the issue fork from social api.

Hi all, I rebased the original issue fork branch so it is up to date and contains the same fixes as the 4.1.x branch, which was confusingly named, and difficult to switch to without running into the issue of a detached HEAD state. I also fixed one little typo. Functionally, both merge requests are the same. 3501411-error-typed-property is just the more standard branch name, is easier to work with, and has the mentioned typo fix. I hope this helps to get this fix merged

Adding a patch

megakeegman → created an issue.

megakeegman → created an issue.

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.