🇺🇸United States @MegaKeegMan

Account created on 8 July 2019, about 5 years ago
#

Merge Requests

Recent comments

🇺🇸United States MegaKeegMan

I have not yet been able to identify what might be causing this. But I will try to reproduce on a fresh installation. Thanks for looking into it.

🇺🇸United States MegaKeegMan

Tested patch on the 2.0.x branch on D10.3 and it looks good

🇺🇸United States MegaKeegMan

I created a simple patch, which should work for anyone using a mailer that incorrectly implements the mail function, like the symfony_mailer contrib module. If Your are using another mailer that correctly implements the mail function, and the mail function returns a boolean as per the Drupal core mail interface, then this patch is not for you.

With that being said, it should be pretty easy to re-patch for that, and I think that patch should also exist.

Unfortunately I don't have a list of which mailer modules do what.

🇺🇸United States MegaKeegMan

Unfortunately, that won't work in the cases where a $mail contains a boolean, which I think it should. But I'd really like to hear a maintainers thoughts on how they want to handle this, and if there is any history behind this.

🇺🇸United States MegaKeegMan

For what it's worth, the MailInterface class in Drupal core hasn't been touched for 3 years, and indicates that the mail function should return a bool, see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

🇺🇸United States MegaKeegMan

Glad to find this issue, as I was also about to report it. It doesn't look like there is any reason this function needs to return an array, since the results are not actually used for anything besides to check if the array returned is empty. Additionally, I can see where some of the confusion is coming from. The mail function, run on line 622 of the .module, is documented to return a bool. However, the implementation that I am seeing from my current sites mailer is called in the MailManagerReplacement class of the symfony_mailer contrib module. So there seems to be a lot of confusion even deeper around whether or not we should be dealing with an array or a bool. I am not sure yet where exactly this confusion stems from, whether this a core change that contrib modules haven't caught up with or whether there are just a few implementations doing it wrong. My best guess so far is that everything should be moving in the direction of the mail function returning a bool, and then any of the code that does anything with the result of the mail function should be updated accordingly. Of course ensuring that our mailers are implementing the mail function correctly. There is also the possibility that the mail function is just documented incorrectly. If anyone has any background knowledge on this, that would be great.

🇺🇸United States MegaKeegMan

One more patch, which just includes one more line that ensures the re-send receipt action appears on the customer receipt viewing page

🇺🇸United States MegaKeegMan

And lastly one more patch. This is exactly the same as 6, but changes the title of the "View PDF" action to "Download PDF". If you do not like this change, please just work from patch 6. However, I had a complaint from a client suggesting that "View PDF" does not indicate to the user that anything will be downloaded. I think it could be argued that this usability improvement fits within the goal of making PDFs viewable by customers, per the title of the issue.

🇺🇸United States MegaKeegMan

Redoing the patch because I noticed a lot of links were broken in patch 5. This patch also adds a route to view_receipt_as_customer, and users that route to generate the link that is included in views. I am not sure if this is what would generally be desired, but it made sense to me and at the very least fulfills my needs.

🇺🇸United States MegaKeegMan

Adding a patch that replaces the admin route to the receipt tab with a non-admin route.

Not ideal yet, since the tab is still in the admin context, but otherwise seems okay for now.

🇺🇸United States MegaKeegMan

I struggled for a bit to understand how to test transactions, and I am wondering if this should be covered in this documentation or if it is covered somewhere else. The few helpful details I learned include:

  • you need to set up a sandbox app, separate from the production app, and make sure to override those values in your staging environment
  • you don't need to create a sandbox buyer account to test transactions, you can just go to the credit card generator under the top level "tools" menu item, and use credit card details from there

I was thinking these details might be helpful to include in this guide, but I did not want to edit the guide directly in case others thought that these were out of scope. In which case, maybe this page should link to some other resource that does include these details.

🇺🇸United States MegaKeegMan

Well I am actually now a little unsure if this is the same issue, but I think it is. I am having this issue only on one of my content types, and it seems to only be breaking while generating sample content for a tag reference field. It is very strange because this same field is on several other content types, but only this one content type breaks. Additionally, it doesn't matter which display I am using. Default breaks just the same as Full, which is too bad because finding this issue had my hopes up that I could just avoid using Default. And lastly, whenever I hit my exception (invalid value) a new tag actually persists, despite the problem being that the value was invalid. Every time layout builder fails to render the preview, I get a new sample tag (taxonomy term).

🇺🇸United States MegaKeegMan

@mlncn I was able to resolve that issue via a patch on https://www.drupal.org/project/drupal/issues/3087632 🐛 MenuLinkContent menu_name max length is too long Needs work
However, I believe that we are running into this issue as well

🇺🇸United States MegaKeegMan

I am working on a site that uses layout builder, and the preview on the manage layout page runs createWithSampleValues which does result in the database error:
String data, right truncated: 1406 Data too long for column 'menu_name'

I applied patch 48 and the error went away. Drupal 10, PHP 8.1

🇺🇸United States MegaKeegMan

This feature actually does work (not all of the details you mention). However, it is worth noting that you can add the receipt field to the orders view and then grant the permission to view order as well as the permission to view receipts. This way, a customer is able to view their receipts. However, like you say, the path for the receipt is under the admin path, and this is very undesirable. A customer should be able to view the receipt under a non-admin path. I would like this very much!

🇺🇸United States MegaKeegMan

Great, thanks for the tip. I will take a look at this and report back here about whether I use this or another approach.

🇺🇸United States MegaKeegMan

Changed all instances of "View display" to "View page", since all displays with a path are page displays, as far as I know. And improved my patch name.

🇺🇸United States MegaKeegMan

Updating the patch for 6.1.x-dev

🇺🇸United States MegaKeegMan

Okay tried reviewing this on 6.1.x and unfortunately does not apply

🇺🇸United States MegaKeegMan

Maybe a big ask but a token would be cool

🇺🇸United States MegaKeegMan

I am trying to use this to generate default content for a distribution though. I could write a script that gets the uuid of the admin, updates the uuid in each default content zip, and then imports. But I guess the alternative is whether the import needs to respect the uuid of the referenced user, or if it could just infer that information based on the user with the given username. Any different ideas or suggestions?

🇺🇸United States MegaKeegMan

Yes. The issue is that the uuid of the admin user is different between instances, so it tries to create a new user with the same name. So I guess I could just update the uuid to reflect the admin on the separate instance. Just a minor inconvenience.

🇺🇸United States MegaKeegMan

The "drutopia_core" issue fork needs review. Please ignore the "drutopia" issue fork (which was created before this issue was moved from the drutopia project queue).

🇺🇸United States MegaKeegMan

MegaKeegMan changed the visibility of the branch 3432238-a-content-editor to hidden.

🇺🇸United States MegaKeegMan

When do you hit this error? It looks like you have the new code deployed. I am not particularly familiar with AEgir, and it is unclear why Drush would be attempting to re-bootstrap the system following database updates. Do you know whether the database updates are finishing?

Here is what I would recommend:

  1. Try performing this outside of AEgir, and see whether you are still running into these issues
  2. Maybe clearly document which build of Drutopia you are moving from and are going to (I realize it is possible some customization has been done)

Otherwise, per the original post: upgrading to Drutopia 2 should be the same as any other upgrade, with one significant caveat: media migration has to be performed - if you want to move to this (e.g. doing a distro-update). We do not (yet) have in place a good media upgrade process. If you want to skip this, you can upgrade to Drutopia 2 without switching/migrating to media entities, but doing a fresh install does force you into requiring a migration.

Another note—the group_permission.calculator service is provided by the group module. I am not sure what is attempting to utilize this service, but my guess is that the group module is not installed, but something else expects it to be, and I am not sure what that something is. Does your site utilize the group module?

🇺🇸United States MegaKeegMan

As I said, I usually perform the distro update, using the module in the comment above to bring in optimizations. There are plenty, but some of the ones that are nice are related to the sites handling of images. A fresh installation of Drutopia 2 comes with media entities. It also includes optimizations of the image styles.

After the distro update, I write a custom module for my project that includes a deploy hook that uses the fieldToMediaEntity function provided by https://www.drupal.org/project/migration_helpers . This just migrates all of the images and/or files to media entities. This method involves a bit of manual work, defining migrations via the parameters of this function call.

Alternatively, you could use https://www.drupal.org/project/image_field_to_media to create the media fields and to migrate the images to media. When I first started using my approach to this, the image_field_to_media module did not support migrating translated media entities, and I had recently done the work of adding this feature to migration helpers. I have not checked whether this has changed. If your site does not utilize translations, this will not be an issue. You could use this module to transition to using media entities, but of course you would not get any other config optimizations without running the distro update.

I intend to take a look at adding such a deploy hook (that utilizes the migration helpers function) to Drutopia. This, of course, would not migrate images that are attached to nodes of any custom content types (or other entity bundles).

🇺🇸United States MegaKeegMan

I just created and related this to an issue about upgrading an existing site to Drutopia 2 / Drupal 10. It can be simpler or more complex, depending on your goals, but feel free to leave any questions you have about this process in that issue.

🇺🇸United States MegaKeegMan

I don't expect Drutopia 1 to run on Drupal 10. Drutopia 2 will run on Drupal 10 though. If you update your existing site, it should continue to work. Updating the config to be up to date with the latest installation config of the various Drutopia modules is a different story.

I have been using https://www.drupal.org/project/config_update to bring in any optimizations to the Drutopia modules. Frankly, this approach is not particularly friendly, and involves a lot of sorting through which config changes you want to keep or get rid of. It is noteworthy that this step is optional. You can switch to Drutopia 2 without attempting to bring in the config optimizations that come with it.

Our intention will not be to continue this approach moving forward, but rather just to write update hooks whenever there are config changes in any of the modules.

🇺🇸United States MegaKeegMan

The documentation on the project page is out of date, and needs to be updated. Please try the following:

composer create-project drutopia/drutopia_template:2.x-dev --no-interaction DIRECTORY (replacing DIRECTORY with your project name)

🇺🇸United States MegaKeegMan

You just have to configure the linkit profile, adding a matcher for media entities.

🇺🇸United States MegaKeegMan

I saw that this document was updated to include a mention of the newer standard of using the .md extension, back in 2022. Is there a reason this was reverted? And can we bring that back? Usage of markdown in documentation is a very nice thing to have, after all.

🇺🇸United States MegaKeegMan

If you use the git command at the bottom of the issue when creating your commit, Drupal credits will be applied properly. Would you like to try again using those details and then force push to the issue fork using the updated commit?

🇺🇸United States MegaKeegMan

Also used this issue to test merge requests. Realized that I had to update the default branch to 1.0.x-dev so that MRs are targeting the correct branch by default.

🇺🇸United States MegaKeegMan

Please commit these changes to the issue fork and open a merge request

🇺🇸United States MegaKeegMan

MegaKeegMan changed the visibility of the branch 3423175-fix-the-issues to active.

🇺🇸United States MegaKeegMan

MegaKeegMan changed the visibility of the branch 3423175-fix-the-issues to hidden.

🇺🇸United States MegaKeegMan

Looks like all of the errors are resolved by patch #4

Thanks for the contribution. I will have to remember to format the code as I build, especially the css, which is getting compiled via sassc

🇺🇸United States MegaKeegMan

Though it is present, I decided to keep open, since this should probably be configured upon installation. Renaming accoringly.

🇺🇸United States MegaKeegMan

Glad to see this coming in. However, I just applied database updates coming from this fix and I notice that the access policy types have not been removed from config. Then this property on the config does not have any affect anymore?

🇺🇸United States MegaKeegMan

Thanks for the info. I will find a time this week to watch the video you recommend. I am sure that implementing tours will be useful.

I have also found access policy types pretty confusing, and it has taken me some time to start picking up on the differences between them and why I would use each of them. While I think my understanding has improved, I still have questions. I think this will be a positive change.

As for the checkmarks. I actually think the checkmarks make sense and send the right message. For whatever reason, I did not understand that the values in the table were affected by configuration of access rules, despite the text saying so. I think what probably happened for me was that I set up my first access policy following along a tutorial, and while following a tutorial my observational powers of what was happening in the UI were a bit lessened, as I was just trying to follow steps, since at the time I lacked the confidence that I could configure the module without a guide. I didn't really start observing what was happening in that table until I started to set up my second access policy.

I don't have a single clear answer on how to resolve this. But whether in a tour or in some referenced tutorial, if while walking through how to set up an access policy there is some suggestion to look at the table before and after a rule is configured, that could help to instill some fundamental understanding for newcomers. Though I wonder how many people use a tutorial or guide initially when installing. I would assume that most people do, but it may not be a safe assumption.

🇺🇸United States MegaKeegMan

The explanation currently says: "Limited means that the role has access but may be constrained by one or more access rules"

This is technically true, but I wonder if instead it should say something like:
"Limited means that the role has access but that one or more existing access rules may apply constraints"

🇺🇸United States MegaKeegMan

Okay. I'm half sorry. I realized that those operations were not limited because I did not have any rules that were related to those operations. I am setting this back to "Active", not because anything needs to be done, but because I would like to take the time to consider whether there may be any UI improvements or simple documentation that could help with this. I was stumped for a bit longer than I am proud to admit, all because of a non-problem. The functionality is there though, and everything seems to be working.

I did not realize that that the access table dynamically checked whether or not their were rules applying to those operations. This makes sense, and is actually a very informative UI when you understand how to read it. Unfortunately, I had interpreted checkmarks to mean that rules CANNOT apply to those operations.

🇺🇸United States MegaKeegMan

Ah hold off on attending to this please. I may be finding answers. Will update shortly.

🇺🇸United States MegaKeegMan

Thinking about how to manage who are group managers and members per group. I think the view bulk operation method you mentioned is definitely worth considering. However, I am considering that simpler approach might be to use https://www.drupal.org/project/cer to sync the entity references between user and group. Will update once I have attempted this implementation. I am wondering how nicely this will work in a many to many situation.

🇺🇸United States MegaKeegMan

No, but if I had a fix I would absolutely contribute it. I have actually switched to using the access policy module for my current project. I was a bit hesitant to use it at first because it is pretty complex, but I am happy to be learning it now. I am glad that you were able to reproduce the issue, though. It is a pretty strange one.

🇺🇸United States MegaKeegMan

For convenience, I am adding this patch that comments out anything related to kss or style sheets.

https://www.drupal.org/files/issues/2024-01-11/remove_styleguide.patch

🇺🇸United States MegaKeegMan

At the time of writing this issue, the module defined a field_social_post_controller. All code related to this field has been removed, and usage of this module now depends on inline_entity_form. Currently, however, the usage and configuration of inline_entity_form is not automated by the module at all. This will either require documentation and/or some more work. Switching to this model means that the post values now only exist on the microblog node.

🇺🇸United States MegaKeegMan

I don't have particularly strong feelings about this, and would be okay with this direction. But I think I prefer postsave. For one, it will be clearer that this occurs later than presave. Additionally, insert, update, upsert, are all database language. Maybe my expectations are off, but I did not find this hook naming to be the most intuitive. I expect this will change between people, but as a Drupal developer I was looking for something closer to Drupal language, and not database language. I understand why insert is called insert, but it did not stand out to me as what I was looking for immediately. I think my prior point is the more important one for me. Though I am stating these preferences, I can appreciate the idea, and ultimately I really just want to see this feature in core, whatever the hook gets called.

Production build 0.71.5 2024