- Issue created by @juagarc4
- First commit to issue fork.
- Status changed to Needs review
6 months ago 9:30am 7 May 2024 - Status changed to Needs work
6 months ago 2:52pm 7 May 2024 - ๐บ๐ธUnited States smustgrave
Adding title update as it doesn't seem to match the change
Tagging for issue summary as proposed solution don't believe matches the change and there are some sections missing from the standard issue template. Would this not be considered an API change? Would add the full template back.
Tagging for sub-maintainer but having a clear issue summary usually helps them.
- Status changed to Needs review
6 months ago 4:57am 8 May 2024 - Status changed to Needs work
6 months ago 6:05am 8 May 2024 - ๐ฉ๐ชGermany juagarc4
Hi smustgrave,
I don't understand why the ticket is not clear enough.
The summary uses the standard template. I only removed the elements not involved in the problem. I can get them back again.
I described the problem I found and ask is there is a good reason for not changing the current implementation by my proposed one.I don't think, the committed solution is the best way to solve the problem either. So I don't understanb what the title and the summary must be adapted to match this solution.
Would this not be considered an API change? I don't think, it's an API change
Maybe I haven't understood you.
- Status changed to Needs review
6 months ago 8:20am 8 May 2024 - ๐ฎ๐ณIndia sukr_s
Changing the proposed solution to check only for permission and not for uid 1 since there is an initiative to remove all dependency / hardcoding on uid 1.
Current MR is based only on permission, so setting back to NR
- ๐ฉ๐ชGermany juagarc4
Hi sukr_s,
Oh, I haven't read about the initiative you mention to remove hardcoded uid 1. But it's for me ok only to check for the permission.
I tried your solutions and now it's working fine if nthe user that access have the permissions assigned. - Status changed to Needs work
6 months ago 12:29pm 8 May 2024 - ๐บ๐ธUnited States smustgrave
Now this ticket seems to be doing 2 things. Either the changes for the user stuff should be reverted or the changes for the migration code.
If it sticks to just user letโs just stick with adding permissions and not additional assertions or test updates
- Status changed to Needs review
6 months ago 4:34am 9 May 2024 - ๐ฎ๐ณIndia sukr_s
I'm not sure I understand. After introduction of the permission, the tests have to be updated otherwise the tests will fail. There is no change in the migration code.
Entity count change for action has changed, not sure why that is so. If you check the history of changes, the action count has been changed multiple times without change in test data. May be subsystem owner can comment on this.
User count will change since we have created new user with the appropriate permission.
Since a new user has been created a conflict will occur in migration and thus an extra step is required.
Let me know if something is not addressed.
- ๐บ๐ธUnited States smustgrave
Still believe additional assertions are out of scope of this issue
Title and summary should still be updated though as the other issues were about removing the admin flag for user tests but this is changing the underlying functionality.
- ๐บ๐ธUnited States mikelutz Michigan, USA
I need to have a conversation with the fms on this to see what we should do exactly. Yes, the new access policy means we shouldn't make uid 1 special anymore, but migrate is a special case. There cannot be any other users in the system prior to a UI upgrade, so the check is actually reasonable in this case, and with migrate_drupal_ui being deprecated and removed from core in the 11.x cycle, the issue may be moot anyway.
Beyond the nonsense of trying to run the UI migrations without a clean database with no entities, we at least know that coming from d7, uid 1 will be an admin, as uid 1 was special in the database we are migrating from. We can't guarantee this on any other users, so the uid 1 check is appropriate in this situation regardless of the new access policy stuff.
- Status changed to Closed: works as designed
6 months ago 2:52pm 9 May 2024 - ๐บ๐ธUnited States mikelutz Michigan, USA
The check against uid 1 is appropriate here, as you cannot run the upgrade path if there are any entities in the system beyond the default ones that exist on install. You can't run the upgrade path as any other user, as your user could be overwritten with a migrated user that didn't have administrator privs, or that wasn't even your account. If migrate_drupal were staying in core in d12, we would have to do some work here to ensure that we were both UID 1 and that UID 1 had an administrator account somehow, which would be difficult as we also couldn't guarantee an administrator role we create wouldn't be overwritten on migration either. Thankfully, we don't have to worry about it since this is all going away, but the use case stated in the original issue summary is invalid.
Since I'm an external worker on the current project I'm working on, I have received an user with the role "Administrator" but obviously it doesn't have the ID 1. So the fact is that I can't execute the upgrades although my user is an admin user.
For this to be true, the system would have to have an additional user created, which means you can't run the upgrades anyway, so the issue is moot.