Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix core depending on theming app #29683

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Nov 12, 2021

This was introduced in 3093548
to fix a bug but I can't reproduce the bug after reverting this change.

Ideally, we would need to create an interface in OCP and cleanup OC_Defaults
instead of depending on OC_Defaults.

This needs testing of the new profile feature with this patch.

Fix #29676

Signed-off-by: Carl Schwan [email protected]

This was introduced in 3093548
to fix a bug but I can't reproduce the bug after reverting this change.

Ideally we would need to create an interface in OCP and cleanup OC_Defaults
instead of depending on OC_Defaults.

Signed-off-by: Carl Schwan <[email protected]>
@MichaIng
Copy link
Member

MichaIng commented Nov 12, 2021

I can confirm this fixes the upgrade issue:

  1. Did a fresh NC22 install
  2. Disabled theming app
  3. Switch to beta channel
  4. Upgrade to NC23 (needed to download and install manually, as occ update:check and web UI didn't offer NC23 yet on beta channel 🤔)
  5. Run web UI (database) upgrade
  6. Reported failure
  7. Applied the changes from this PR
  8. Rerun web UI upgrade
  9. Success

I guess in line 409:

* @return string SCSS code for variables from ThemingDefaults

should be updated as well to:

* @return string SCSS code for variables from OC_Defaults

EDIT: Done with 9189bc2 reverted to exact state from before: 3093548#diff-709880c12d31976830cbebd16b0ccb48a0a4f6d611d4896e2c56240a196bdfeb

The original reason for this change and the problem with it were recognised already: #28751 (comment)
Looks like it was fixed here?

Just recognised another issue, as the theming settings are still shown even if the theming app is disabled. Applying any changes of course fails with following browser console errors:

jquery.js:9600 POST http://192.168.1.23/nextcloud/index.php/apps/theming/ajax/updateStylesheet 412 (Precondition Failed)
send @ jquery.js:9600
ajax @ jquery.js:9206
e.ajax.e.ajax @ jquery-migrate.min.js:2
C.each.C.<computed> @ jquery.js:9355
setThemingValue @ settings-admin.js?v=5e730a6d:29
(anonymous) @ settings-admin.js?v=5e730a6d:197
u @ jquery.js:3534
c @ jquery.js:3602
setTimeout (async)
(anonymous) @ jquery.js:3640
l @ jquery.js:3268
add @ jquery.js:3327
(anonymous) @ jquery.js:3660
e.Deferred.e.Deferred @ jquery-migrate.min.js:2
then @ jquery.js:3645
onChange @ settings-admin.js?v=5e730a6d:196
dispatch @ jquery.js:5183
g.handle @ jquery.js:4991
msg.js:86 Uncaught TypeError: Cannot read properties of undefined (reading 'message')
    at Object.finishedAction (msg.js:86)
    at Object.finishedSaving (msg.js:69)
    at Object.<anonymous> (settings-admin.js?v=5e730a6d:35)
    at l (jquery.js:3268)
    at Object.fireWith [as rejectWith] (jquery.js:3398)
    at k (jquery.js:9307)
    at XMLHttpRequest.<anonymous> (jquery.js:9548)
finishedAction @ msg.js:86
finishedSaving @ msg.js:69
(anonymous) @ settings-admin.js?v=5e730a6d:35
l @ jquery.js:3268
fireWith @ jquery.js:3398
k @ jquery.js:9307
(anonymous) @ jquery.js:9548
load (async)
send @ jquery.js:9567
ajax @ jquery.js:9206
e.ajax.e.ajax @ jquery-migrate.min.js:2
C.each.C.<computed> @ jquery.js:9355
setThemingValue @ settings-admin.js?v=5e730a6d:29
(anonymous) @ settings-admin.js?v=5e730a6d:197
u @ jquery.js:3534
c @ jquery.js:3602
setTimeout (async)
(anonymous) @ jquery.js:3640
l @ jquery.js:3268
add @ jquery.js:3327
(anonymous) @ jquery.js:3660
e.Deferred.e.Deferred @ jquery-migrate.min.js:2
then @ jquery.js:3645
onChange @ settings-admin.js?v=5e730a6d:196
dispatch @ jquery.js:5183
g.handle @ jquery.js:4991
jquery.js:9600 POST http://192.168.1.23/nextcloud/index.php/apps/theming/ajax/updateStylesheet 412 (Precondition Failed)
send @ jquery.js:9600
ajax @ jquery.js:9206
e.ajax.e.ajax @ jquery-migrate.min.js:2
C.each.C.<computed> @ jquery.js:9355
setThemingValue @ settings-admin.js?v=5e730a6d:29
(anonymous) @ settings-admin.js?v=5e730a6d:206
u @ jquery.js:3534
c @ jquery.js:3602
setTimeout (async)
(anonymous) @ jquery.js:3640
l @ jquery.js:3268
add @ jquery.js:3327
(anonymous) @ jquery.js:3660
e.Deferred.e.Deferred @ jquery-migrate.min.js:2
then @ jquery.js:3645
onChange @ settings-admin.js?v=5e730a6d:205
dispatch @ jquery.js:5183
g.handle @ jquery.js:4991
msg.js:86 Uncaught TypeError: Cannot read properties of undefined (reading 'message')
    at Object.finishedAction (msg.js:86)
    at Object.finishedSaving (msg.js:69)
    at Object.<anonymous> (settings-admin.js?v=5e730a6d:35)
    at l (jquery.js:3268)
    at Object.fireWith [as rejectWith] (jquery.js:3398)
    at k (jquery.js:9307)
    at XMLHttpRequest.<anonymous> (jquery.js:9548)
Enabling the theming app of course fixes it. Now I wanted to check whether it works with guest accounts, but I cannot find the way how to create a guest account 😅.

@szaimen szaimen removed their request for review November 12, 2021 16:07
@MichaIng MichaIng modified the milestones: Nextcloud 23, Nextcloud 24 Nov 12, 2021
@MichaIng MichaIng self-requested a review November 12, 2021 18:24
@MichaIng
Copy link
Member

/backport to stable23

@CarlSchwan CarlSchwan requested a review from artonge November 16, 2021 10:01
@artonge
Copy link
Contributor

artonge commented Nov 16, 2021

A linter rule preventing usage of use OCA\* in core PHP files could be nice to have. Could this be possible?

@CarlSchwan
Copy link
Member Author

A linter rule preventing usage of use OCA\* in core PHP files could be nice to have. Could this be possible?

That would indeed be a good idea, but I'm not sure how feasible it is. I could read the php-cs-fixed doc

@CarlSchwan CarlSchwan merged commit 6ea81dd into master Nov 16, 2021
@CarlSchwan CarlSchwan deleted the bugfix/29676/fix-theming-dependencies branch November 16, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[23.0.0 RC1] Upgrade fails if theming app is disabled
4 participants