#Novice

I made some basic cosmetic changes to the product details page, I made it as basic as possible for the admin to customize as desired, but the current styling/theme of the product details page works for many cases.

I attached the files, I leave it to "Needs Review" for a while just in case anyone wants to get extra credits !

That's a good start, but we can't just remove constants, because people will be using them.

We have to deprecate them: see https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

Removal will come in a future version.

While we're at it, I think that the wording from node is clearer:

> * Denotes that the [node] is not published.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Feedback and summary and are still incomplete.

Also 1 MR should be closed

I was facing this same issue but after applying patch #75 works for me.

Thank you!

Hello @sd9121 any luck on this? I think I'll create a new tagged release as it is now and then do the additional fixes, I just want to deliver something that the user can look, play and customise it.

I will try to fix it to next days.

Automatically closed - issue fixed for 2 weeks with no activity.

Hello @sd9121 , how's it going with this? I committed some config changes and it should be enough for now, after that the webadmin / webmaster can change the commerce settings as desired:

https://www.drupal.org/project/sshop/issues/3523670 🐛 Save changes done to the theme files (config/templates) for the commerce blocks and other views Active

Automatically closed - issue fixed for 2 weeks with no activity.

Thanks so much @mradcliffe and @kristiaanvandeneynde — that clears things up perfectly!

I’ll make the adjustments now based on the updated direction: using access site reports, removing the UID 1 check, and updating the test.

Really appreciate your help — I’ll update the MR soon!

do we know if a contrib module could be using this?

I was testing my API with Bruno when this became an issue for me too. In my case at least, the comment at https://drupal.stackexchange.com/a/307648 led me to the solution, i.e. Bruno now stored the Drupal session cookie and because of this, also expected the CSRF in the header, even though it didn't before when using Oauth.

So should this one and the taxonomy one be postponed till an agreed upon structure can be decided?

I updated the issue summary to include how to resolve and removed the tag. I left the Novice tag because I think we have some consensus on the issue.

Having it check for the "access site reports" permission would also be an acceptable solution to me. The essence is that the UID 1 check needs to go and preferably be replaced with a permission check.

Automatically closed - issue fixed for 2 weeks with no activity.

I don't think we need a new permission, "access site reports" permission fits because the report is already accessible by users who have access to dblog (or syslog, jsonlog) as MigrateController is a redirect.

If we added a new permission, then a non-admin user would need both that permission and "access site reports" to access a filtered list of migrate_drupal_ui.

I've created a patch that preserves the teaser break delimiter (<!--break-->) when using the Basic HTML text format.
This approach uses a placeholder tag during filtering and restores the original delimiter afterward, without modifying the core HTML filtering process.

The whole point of these issues is to make sure core no longer hard-codes anything based on the idea that user 1 is special. There are plans to remove user 1's special behavior in a future major Drupal release. So we really can't leave this test to rely on UID 1.

This access check explicitly allows only UID 1:

So that is what we need to change: Introduce a new permission and make the route check for said permission. Admins (including UID1) will automatically get the new permission, so a simple change record detailing that non-admins who want access need to be assigned the permission should do.

Here's the explain on the query from loading the view and clicking the tablesort.

MySQL [local]> explain SELECT node_field_data.nid AS nid, users_field_data_node_field_data.uid AS users_field_data_node_field_data_uid
    -> FROM
    -> node_field_data node_field_data
    -> INNER JOIN users_field_data users_field_data_node_field_data ON node_field_data.uid = users_field_data_node_field_data.uid
    -> WHERE (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1)
    -> ORDER BY users_field_data_node_field_data.name ASC
    -> LIMIT 50 OFFSET 0
    -> ;
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
| id | select_type | table                            | partitions | type | possible_keys                                | key     | key_len | ref                       | rows | filtered | Extra                           |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
|  1 | SIMPLE      | node_field_data                  | NULL       | ALL  | node_field__uid__target_id                   | NULL    | NULL    | NULL                      |    1 |   100.00 | Using temporary; Using filesort |
|  1 | SIMPLE      | users_field_data_node_field_data | NULL       | ref  | PRIMARY,user__id__default_langcode__langcode | PRIMARY | 4       | local.node_field_data.uid |    1 |   100.00 | NULL                            |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
2 rows in set, 1 warning (0.000 sec)

So yeah we're using temp tables.

Setting to PMNMI for the decision on whether we want to do this. Personally I'm happy to close.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Yes, but why do we even need it to have the special migrate access check?

If the point is to reduce the tests that depend on this special case, then maybe fixing migrate_drupal_ui.log route to use that permission instead would make sense because an administrative user with "access site reports" already has access to it?

Hi @mradcliffe, thanks for highlighting that.

To clarify why this test still requires super user access:

Although the test doesn't perform migration actions itself, it accesses the /admin/reports/upgrade route. That route — as defined in migrate_drupal_ui.routing.yml — uses a _custom_access requirement pointing to \Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess().

This access check explicitly allows only UID 1:

return AccessResultAllowed::allowedIf((int) $account->id() === 1);

o even just viewing the upgrade report (MigrateController::showLog) fails unless the test runs as the root user. Removing usesSuperUserAccessPolicy = TRUE causes an access denied error.

That’s why in the MR, we restored the flag and removed only the outdated @todo. A broader solution (e.g., updating that route to rely on a permission like access site reports) would be ideal, but it's out of scope for this issue.

Happy to help push that discussion further if needed.

Thank you!

I think we should answer the question as to why this test needs super user access when the test is not asserting anything about migrations and why the reports needs the special migrate access check prior to reverting the commit.

All of my questions have been answered. All of the replacements line up and the new landing places make sense to me!

Hi,
I've rebased the branch onto the latest 11.x and fixed the coding standard issues.

All tests are now passing.

Please review when you have a moment, and feel free to let me know if anything else need.

Thank you!

Fixed on both the 3 and 4 branches.

Hi, could you please review this MR !11645?

I've restored the usesSuperUserAccessPolicy = TRUE property and removed only the outdated @todo, as discussed.
This change reflects the current behaviour where Migrate UI still requires user 1.

Please feel free to review, and if anything needs to be adjusted or clarified, please let me know!
Thank you!

Hi, I've created a new clean branch to address this issue.

The previous branch had too many unrelated commits and was outdated, causing multiple conflicts and making it hard to review.
This new branch includes only the intended change: replacing all usages of the deprecated assertWaitOnAjaxRequest() with assertExpectedAjaxRequest() across the core/ directory.

A new merge request has been opened from this clean branch.

Could someone please review and test this updated MR !12108?
Thank you!

Committed. Thanks!

Thanks @sd9121 I'll create a few more issues and then we are ready for the BS5 migration and Drupal Recipes.

Thanks !

@sd9121 bear in mind that I still need to commit some config changes in order for you to work on this, see this issue 🐛 Save changes done to the theme files (config/templates) for the commerce blocks and other views Active

I'll look on that shortly

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Committed ab9823e and pushed to 11.x. Thanks!

nod_ → closed merge request !11620

Hi @danrod,
Yes, I'd be happy to help with testing and working on more issues for this theme before the tagged release. I'm also interested in supporting the migration to Bootstrap 5 and the shift to using Drupal recipes.

It looks like I didn't close this.

It is strange, I will post a video, when I click on the mobile menu, it collapses out as expected but then it goes back to the intial state (the menu collapses in).

May be the right status?

Hi @sd9121 thank you, It worked like a charm:

I'll mark this as "Fixed" and give you the credits of course.

Would you like to help testing and working on more issues on this theme before launching a tagged release? I'd like to have this before migrating this theme to Bootstrap 5 and using Drupal recipes instead of theme features.

sd9121 → closed merge request !39

Automatically closed - issue fixed for 2 weeks with no activity.

Thanks @sd9121 ,

Would it be possible to add this to the issue fork sshop-3523794 and create a MR, please?

Thanks !

Hi,
I have attached a patch that fixes the issue with long menu item text not wrapping correctly. This ensures that text in the menu wraps onto the next line instead of overflowing or breaking the layout.

Hi @danrod,

I tested the mobile menu using "drupal/estore": "2.2.x-dev@dev" on both Drupal 10 and 11, and it's working fine — the menu toggler opens and remains open as expected.

Could you please provide more information about:

The version of drupal/estore you used?

The browser/device where the issue occurred?

This will help narrow down the cause. Thanks!

After about 9 months there has been no response so state that this is still needed. Therefor, closing.

If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.

Automatically closed - issue fixed for 2 weeks with no activity.

I'll merge the changes to the 8.x-1.x branch to fix other issues that depend on this.

I applied some misc config/theming/styling changes and the single node page looks much better:

I'll move it to "Needs Review", just in case anyone want to have look for it (Novice users)

Thanks!

I'll merge this file to the default branch.

Done, please review again, sorry for the commit comment, the $defaults string was replaced with an empty string.

It's used in the parent class -- see DefaultPluginManager::processDefinition().

Come to think of it, the correct docs here would be an inherit doc tag, which is what inherited properties should have. That way it's clear that it's coming from a parent class.

I add a MR for this with the config changes and I can see that the field is mandatory and the text is displayed in the blog teasers:

Ready for review.

Hello @joachim I actually deleted the $defaults property and its docs, I don't see it being used anywhere in the class (or module). Please review.

This was committed to Drupal 8. I am restoring the meta data to that time.

Automatically closed - issue fixed for 2 weeks with no activity.