🇩🇪Germany @Anybody

anybody → created an issue.

anybody → created an issue. See original summary → .

Seems I forgot to tag a new stable release!

Upgrading this to D11 compatibility, which is needed now!

anybody → created an issue.

Maybe try https://www.drupal.org/project/entity_add_another → ?
The maintainer here seems to be inactive

anybody → created an issue.

I contacted https://www.drupal.org/u/kongkx → via contact form now, hoping for a reply and merge here.

I contacted @geoanders new via contact form. @omarlopesino did you get in touch with any of the maintainers?

@albeorte, could you please tag a new stable release compatible with Drupal 11? Currently only the dev is compatible!

@rwam what you write sounds like you used a hack? You can simply leave the label empty!

anybody → created an issue.

anybody → created an issue.

(You may use tugboat for review / testing)

Here we go :)

@grevil: Could you please add a new branch and MR against 2.x here too? Thanks! (Birapa)

@grevil could you please add a separate branch & MR against 2.x? Thanks!

Sure! :)

Thanks @tostinni! Great work!
As this is risky to break things for the different constellations, it would be super helpful to have some tests, please.

Please also note related core issues, if you think you may have run into this: 🐛 Untranslated menu items are displayed in menus Needs work (While I don't think this issue is the case here)

If you agree @smustgrave, we might implement this as MR for the future

anybody → created an issue.

Nice @miroslavstankov that looks good!

I think we should consider showing this option only, if content translation module is enabled? And the default_value should fallback to FALSE, if not set?

If possible please add a follow-up for tests, currently that seems to be missing entirely...

Totally agree @ressa! Would be great if we could find some more active maintainers (maybe from Acquia or other large partners or long-term Drupal contributors...)

Thank you!!

Thanks, yes and now with 2.2 you can add infinite additional tags :)

So this is fixed through 💬 Creating custom meta tag Active 🎉🎉🎉
Just tried it.

GREAT, this seems to be fixed in 2.2 as a release highlight: 💬 Creating custom meta tag Active

Let's check if it's possible as expected now.

@damienmckenna this is still open, should just be a simple GitLab settings change?

@jrochate you mean the metatag_custom_tags submodule? Or a contrib module like https://www.drupal.org/project/custom_metatag → or https://www.drupal.org/project/custom_meta → (is the submodule now).

Could you please be more specific and link it here?

Does
https://git.drupalcode.org/project/metatag/-/blob/2.2.x/metatag_custom_t... solve this eventually already? (from metatag_custom_tags submodule)

@thomas.frobieter this is already possible, as @grevil found out. Here's a nice code example how to get the steps once the tour is started:
https://git.drupalcode.org/project/tour_extras/-/blob/1.x/modules/tour_e...

Hope that tag is appropriate?

FYI - just ran into the same client request in a further project. The completed order was refunded and the client was wondering why the order state couldn't be changed to "Canceled".

Instead of defining a new workflow I decided to add a translition using hook_workflows_alter() in a custom module:

/**
 * Alter workflows.
 *
 * @param array $workflows
 *   Workflow definitions, keyed by plugin ID.
 */
function hook_workflows_alter(array &$workflows) {
  // Allow cancelling completed orders:
  // @see https://www.drupal.org/project/commerce/issues/3381411
  $workflows['order_default']['transitions']['cancel']['from'][] = 'completed';
}

Since we are already using "token_clear_cache()" inside the normal "TokenCustomForm", I'd say we merge this as is. If it causes any trouble in the future, we can look further into it.

Fine with that!

Whao @smustgrave (at least for now!) it seems fixed through 🐛 Broken when using JS aggregation Fixed !!

Would be great, if you could tag a new release, once the other fixes are in, then I'll try again with the latest stable, but looks good! So I'll close this as duplicate for now and if it should happen again despite the fix, let's reopen it!

@grevil: I think you're wrong, the + is just a bit misleading...

See
_permission: A permission string (for example _permission: 'access content'). You can specify multiple permissions by separating them with ',' (comma) (_permission: 'access content,access user profiles') for AND logic or '+' (plus) for OR logic (_permission: 'access content+access user profiles' means a visitor needs either the access content permission or the access user profiles permission to view the page.)
https://www.drupal.org/docs/drupal-apis/routing-system/structure-of-routes →

Nice, thank you!!

Parent issue is fixed now!

Nice, thanks @smustgrave!!

Nice @smustgrave! Maybe add a comment why we're doing that? I don't think it's self-explaining.
I'm fine with that, while it probably won't hit 100% ...

@smustgrave nice idea, we'll check that! Logically, I'd say this can still happen, but maybe not that often... let's see!

anybody → created an issue.

@macsim feel free to prepare a MR with that functionality or solve it in contrib / custom. Seems a very edge-case?

This is a votingapi issue as the query clearly shows

Thanks, I understand the use-case. Improved this a bit, but still the tests need to address the new permission.

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

@iheb.attia I think instead of the function it should use the TokenService and ->clearCache() using dependency injection:

$this->tokenService->clearCache();

Please create a MR instead of using patches.

@grevil and @maintainers: Shouldn't this be major, as it breaks widely used field_group functionality and is super hard to find as root-cause?

For us this happens for paragraphs which we're using as Mega Menu elements in Menu items. I can confirm the patch in 📌 MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields Needs work solves it. Does the module need any further changes?

This issue could be used to add a test for this case (which should be failing until it is resolved).

Setting priority to Normal based on the further effect not only on logs, but also on the translation server, mentioned by @heddn in #37. Thanks for the ping @ressa!

Before the issue summary update we should decide on the final way to go, after one more was suggested in #43. Would be great to get some final feedback from the others here, what's probably best.

Thanks, I removed the @todo!

Regarding BigPipe: Yes and the other part would be a test with and without aggregation. With the fix it should be green, without not... Do you have any ideas regarding that? (#9 )

Still I'm fine to leave that out, at least for now with the fix, just saying because BigPipe addition is similar...

Ups sorry YES, our fault -.-

Let's wait until we use it. In general the improvements from the Navigation Core module look nice!

anybody → created an issue.

Isn't this outdated now that Tour is a contrib module? 📌 Deprecate and remove shepherd.js Active
https://www.drupal.org/node/3442299 →

Otherwise please reopen!

Thanks @grevil! I'm 50/50...

I'm totally sure that ID, title, etc. should go into the base class!
I'm a bit unsure about:

• body
• selector
• position

because they might be required for 90% of all Tip Plugins and 95% of all "content" Tip Plugins.

Your point with the schema on the other hand is totally valid, looking at that, I'd definitely vote to unify all into the Base.
An alternative would be to put those three into a trait, sth. like src/TipPluginContentTrait.php but for the schema that would still mean, that these three should be moved out of the general tip and into tour.tip.text.

What do you think @smustgrave?

Thanks @grevil for adding that flexibility! Definitely super helpful for contrib / custom cases, where this flexibility is needed. Together with plugins this should make integrations super flexible :)

RTBC once finished!

@joao.ramos.costa definitely yes! No idea why that didn't happen. Typically, I do that... Some days ago the functionality was broken for me, perhaps that caused it.

anybody → created an issue.

Screenshots for data array documentation, if we should run into this again.

Replaced the old condition together with @thomas.frobieter, documented possible alternative(s) and added the new, hopefully more reliable, condition!

Replaced the old condition together with @thomas.frobieter, documented possible alternative(s) and added the new, hopefully more reliable, condition!

anybody → created an issue.

anybody → created an issue.

Here we go :)
I think that will be helpful and won't do any harm.

anybody → created an issue.

can we remove the todo and I'll merge.

Mhm but shouldn't we better keep it, as it shouldn't be there forever, due to its workaround nature?

And unless it becomes a major problem not sure we need to break up how it’s compiled. I’m 90% positive moving to it’s own library and loading like it did in core no longer works with v13 of shepherd. Core was using v10

Should we give it a try in a separate issue and see if it works? Shouldn't be too hard, I guess?

And the weight was added because in 11.1 asset ordering was changed and tour completely stopped working because it was loading last

MIGHT be that this could also be resolved in the separation issue, if the reason is the library bundling?

In 📌 Update shepherd.js library Active @darvanen wrote:

It is possible to include the shepherd library without using a bundler but ensure that it loads before the tour code is not something I am familiar with

When using it from CDN or library, having the library as dependency of the tour initialization should happen like for any other JS?

Thank you both! We should definitely postpone the stable release until all features are present again. What about just keeping the 2.x (dev) branch for now and not releasing anything? Who needs it, can use it and we don't confuse people.

Ne @miroslavstankov you're right, my bad! Nice new tests, RTBC from my side!

Thanks @smustgrave what do you think, should we merge the workaround for the meantime and open a separate issue to check the library separation? Before that we should definitely check the reasons it was done this way in 📌 Update shepherd.js library Active !

Fix attached as static patch until this is finally solved :)

Thanks @smustgrave sadly the reordering doesn't fix it. But we just found out that preprocess: false fixes the issue. As long as dist/tour.js is not part of an aggregated js file, it's fine!

I could imagine that this is a combination of different things:
1. "weight" property: I don't know why weight: -1 is used, but it would be great to tell why (and add a comment for the future). If it's not needed, it should be removed. Maybe it just runs to early as part of the Drupal aggregate?
2. The build/tour.js compiled source pollutes the global scope with a lot of functions. Aggregated with other JS in the same JS file, that may conflict if any other module minifies to the same variable names!
The code looks like this:

/*! shepherd.js 13.0.3 */ function tn(e) {
    return e instanceof Element;
}
function ke(e) {
    return e instanceof HTMLElement;
}
function Z(e) {
    return typeof e == "function";
}
function Ce(e) {
    return typeof e == "string";
}
function T(e) {
    return e === void 0;
}
[...]

Possible solutions:
1. Use the preprocess: false quickfix from the MR. I'd even vote to merge it, until we clearly know how this happens and how to fix it. The impact should be minimal, but it removes the side-effects. Maybe what we see is even a side-effect of 🐛 Only file JavaScript assets with preprocessing enabled can be optimized. Active but I don't think so.

2. Do not pre-built the sources and do not aggregate tour.js with the library in dist. Imho that is not Drupal best-practice and may lead to such side-effects. Instead use the libraries functionality from Drupal for local sources or CDN: https://docs.shepherdjs.dev/guides/install - if the maintainer agrees, let's do that in a separate issue. There are further benefits, for example the user can independently upgrade to a (paid if commercial) Shepherd v14. As long as the modules tour.js is bundled, that seems impossible.

Short-term I'd vote for (1) and long-term for (2)! :)

@grevil: Could you please check if it's possible to subscribe on the step events from Shepherd.js within

$(document).on('drupalTourStarted', function() {
  // tour == Shepherd
  let tour = Drupal.tour.get();
  // @todo: subscribe to tour events
});

If yes, we should add some of these snippets to the README for the Shepherd tour and step events! Then it wouldn't need further triggers here.

PS: please note that you may probably need to use jQuery, as I think to remember that $().trigger doesn't trigger native JS events. Maybe we could also add a vanilla js trigger besides the jQuery one, if I'm correct.

anybody → created an issue.

Thanks @grevil yes totally makes sense architecturally, also to allow clean plugins in contrib.
I think it just wasn't needed in the past, because there is only one plugin. But clear to see these things are very general :)

anybody → created an issue.

anybody → created an issue.

commented

Nice @lrwebks! Merging!

Ideas welcome!

anybody → created an issue.

@damienmckenna sorry for the ping, but could you maybe merge this and the other major RTBC issue and tag a new release?

Maybe someone could try contacting the maintainers to merge this? Thanks!

@timmy_cos thanks for the information. Does the MR here solve it for all described cases?

@damienmckenna looks like the merge target needs to be updated to 2.2.x?

Some tests are still failing, but at least they look related... -.-

PS: Just saw that your comment was before the latest MR that started fresh in #74 that doesn't use the path_alias module anymore and instead uses OutboundPathProcessorInterface and others, which looks good! Maybe the comment is already outdated, we just need to finish MR!7619 (and its tests)? That would be lovely.

@berdir in #66 you wrote

I think this should not be in the path_alias module, which is now technically optional although most people are using it. It should be a separate event subscriber in core.

and in #69 @grevil asked:

What's everyone's opinion on @berdir's comment in #66? What would be a good place for this Event Subscriber, if we would move the implementation to core?

We're running into this again and would finally like to fix it in core, but it would be sad if it goes into the wrong direction.

Do you have a specific idea where this should be implemented cleanly? In #77 I thought about OutboundPathProcessorInterface. If you could give us a clear direction, @grevil could try to solve this tomorrow. Feel free to contact him in Slack or write here if you (or any other Core-expert) has some minutes.

Thank you so much! Hopefully this is a good chance to get this DONE :)

I can confirm that the typical reason for this issue is an empty path alias of a node. So this is a result of that root cause, but can happen, as we see above.

For example we set "/" for the front page alias and after saving the node twice, the URL alias is empty and this happens... That should be fixed in: 🐛 url() should return / when asked for the URL of the frontpage Needs work . Maybe it even fixes the main root cause of this issue. But I guess other cases can also lead to an empty URL alias and cause this.

anybody → created an issue.

Code-wise LGTM and works as expected, has tests, what else do we want :)

Finally I just left some comment questions asking if we should add a consistent @deprecated comment everywhere "theme" is still present in code, so that it can be easily and consistently removed in D12 or would that just bloat the code?

Settings this RTBC anywhere for the maintainers etc. to look into this and decide.

Thank you all!

PS: I'd also still consider this a major UX bug, just like @chi in #3. From my perspective, the condition was not really usable.