[#39431] - [4.3] Restore js files deleted with PR's #38823 and #39374 in order to be b/c for layout overrides
- Fixed in Code Base
- 17 Dec 2022
- Medium
- Build: 4.3-dev
- # 39431
- Diff
- richard67:4.3-dev-restore-deleted-assets-for-bc
User tests: Successful: Unsuccessful:
Pull Request for Issue #39408 .
Replacement for PR's #39408 and #39413 .
Summary of Changes
This pull request (PR) adds back the javascript files which have been removed by PR's #38823 and #39374 because they might still be used in layout overrides.
This also fixes the currently failing versioning step of the package build which currently also causes the 4.3 nightly builds to fail.
That was caused by PR #38823 not having them removed from file build/media_source/com_users/joomla.asset.json.
So this PR doesn't need to modify that file, it only needs to add back the js files from that PR, while from the other PR it also need to add back the assets to file build/media_source/com_templates/joomla.asset.json.
A @deprecated comment is added to the top doc block of each restored js file.
Testing Instructions
Test 1 - Package Build
It requires a development environment (Git clone of this repo, composer, npm) either on Linux or on Windows with WSL to run the build.php script.
- Checkout the current 4.3-dev branch where #39374 has been merged in on December 13, 2022.
- Run
php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2.
Result: See section "Actual result BEFORE applying this Pull Request" below. - Clean up everything with
git clean -d -x -fandgit checkout .. - Checkout the branch of this PR. You can do that with commands
git fetch upstream pull/39431/head:test-pr-39431and thengit checkout test-pr-39431. - Repeat step 2.
Result: See section "Expected result AFTER applying this Pull Request" below.
Test 2 - Updating from 4.2.6 with Layout Overrides
-
Install Joomla 4.2.6.
-
Create following layout overrides:
- com_templates/template
- com_users/mail
-
Update to the patched package for this PR by using this custom update URL https://ci.joomla.org/artifacts/joomla/joomla-cms/4.3-dev/39431/downloads/60159/pr_list.xml or this package download https://ci.joomla.org/artifacts/joomla/joomla-cms/4.3-dev/39431/downloads/60159/Joomla_4.3.0-alpha2-dev+pr.39431-Development-Update_Package.zip .
Result: The update succeeds. -
Verify that the layout overrides which you have created in step 2 are still working as before.
Result: The layout overrides work as before.
Actual result BEFORE applying this Pull Request
Test 1 - Package Build
At the end of the output in the command window:
> joomla@4.0.0 versioning
> node build/build.js --versioning
[Error: ENOENT: no such file or directory, lstat '/home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670929783/media/com_users/js/admin-users-mail-es5.min.js'] {
errno: -2,
code: 'ENOENT',
syscall: 'lstat',
path: '/home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670929783/media/com_users/js/admin-users-mail-es5.min.js'
}
`npm run versioning` did not complete as expected.
Expected result AFTER applying this Pull Request
Test 1 - Package Build
At the end of the output in the command window:
> joomla@4.0.0 versioning
> node build/build.js --versioning
Timer: Versioning finished in 160 ms
Cleaning checkout in /home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670930708.
Cleaning vendors.
Cleanup complete.
Generating optimized autoload files
Generated optimized autoload files containing 2381 classes
Cleaning Composer manifests in /home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670930708.
Cleanup complete.
Workspace built.
Create list of changed files from git repository for version 4.3.0-alpha2-dev.
Delete folders not included in packages.
Build full package files.
Building Joomla_4.3.0-alpha2-dev-Development-Full_Package.zip... done.
Build full update package.
Building Joomla_4.3.0-alpha2-dev-Development-Update_Package.zip... done.
Build of version 4.3.0-alpha2-dev complete!
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
The deprecation of these js files has to be documented on manual.joomla.org . But since it was PR's #38823 and #39374 which made these files obsolete and so deprecated, it is not subject of this PR here to do that. Clarifications among maintainers is ongoing, and the deprecations will be documented at the end.
joomla-cms-bot
- | Category | ⇒ | Repository NPM Change JavaScript |
| Status | New | ⇒ | Pending |
| Labels |
Added:
Release Blocker
NPM Resource Changed
PR-4.3-dev
|
||
| Title |
|
||||||
The deprecation of these js files has to be documented on manual.joomla.org . I need help with that because I don't have any experience with that. At least an existing example would be helpful.
@Hackwar already opened a PR for the manual but it is incomplete: joomla/Manual#68
and @brianteeman needs to add the deprecation on the manual too
Thanks @drmenzelit . I've decided to change the documentation part of this PR's description so it's more clear ;-)
And also @drmenzelit you should rename the js as I suggested in #39161 because your changes there fall into the same category
I have tested this item
For the record, I'm not necessarily endorsing the idea of shipping dead code...
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39431.
For the record, I'm not necessarily endorsing the idea of shipping dead code...
@dgrammatiko Me neither. But what should I do? Thanks a lot for testing.
and @brianteeman needs to add the deprecation on the manual too
no i dont need to do anything
And also @drmenzelit you should rename the js as I suggested in #39161 because your changes there fall into the same category
on my PR, that has to be reworked anyway, the change on the name of the js file is due to changes in the dependency....
the change on the name of the js file is due to changes in the dependency....
@drmenzelit as I already wrote there the nodejs build tools allow you to rename the distribution files
the change on the name of the js file is due to changes in the dependency....
@drmenzelit as I already wrote there the nodejs build tools allow you to rename the distribution files
ok, thanks. I couldn't find your comment on the PR...
I have tested Test 1 OK.
Test 2: I cannot figure out why there is nothing in the diff editor after update for template overrides. The default behavior is that the diff editor gets the original file. I thought it was because of the CSS file changes.
I have tested Test 1 OK.
Test 2: I cannot figure out why there is nothing in the diff editor after update for template overrides. The default behavior is that the diff editor gets the original file. I thought it was because of the CSS file changes.
Take the PR as it is or forget it and have a broken build. The pr fixes that and makes the overrides not fail with an exception. If something else doesn’t work as before, I don’t care. Find someone else who fixes that. I have done what I can to fix what others have broken.
Update: The failure with exception was the other override for the mass mail thing. But I‘m tired and won’t invest more time.
Take the PR as it is or forget it and have a broken build. The pr fixes that and makes the overrides not fail with an exception. If something else doesn’t work as before, I don’t care. Find someone else who fixes that. I have done what I can to fix what others have broken.
Update: The failure with exception was the other override for the mass mail thing. But I‘m tired and won’t invest more time.
I was not trying to offend you by any means Richard @richard67. I just wanted to make sure the behavior I encountered is 'ok' by adding my comment, so that Dimitris, or someone else testing the PR, can confirm it's ok.
Thank you Richard for your investment, it's been appreciated.
I understand, Richard @richard67, no need to apologize. I followed the conversations and it was a lot, indeed. There are decisions to make and people have different opinions. You've been very helpful in fixing the issues encountered, thank you
I have tested this item
Regardless of my previous testing comment, this PR does produces what it advertises.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39431.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39431.
@obuisard I can confirm that the diff view is not shown when using the toggle button to show it. To make that work with a layout override from 4.2.x it could require modifications of the deprecated js file. It seems to use local storage in the event handler, and there might have been some changes about that. It would need to check the details but I have to do something else soon. I think that could be fixed with a follow up PR. Other stuff seems to work in the override edit view.
| Labels |
Added:
?
|
||
@obuisard I can confirm that the diff view is not shown when using the toggle button to show it. To make that work with a layout override from 4.2.x it could require modifications of the deprecated js file. It seems to use local storage in the event handler, and there might have been some changes about that. It would need to check the details but I have to do something else soon. I think that could be fixed with a follow up PR. Other stuff seems to work in the override edit view.
Thank you for checking it out Richard @richard67.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-12-17 13:40:49 |
| Closed_By | ⇒ | obuisard |
Thank you Richard @richard67 for your help in the matter and your patience throughout the discussions.
Possibly we should implement a special check in the override edit view if it has an updated override for itself, and if so, show some kind of alert in the edit view that it might not work because of that reason.
Please don't, if there's a regression open an issue and assign it to me.
Possibly we should implement a special check in the override edit view if it has an updated override for itself, and if so, show some kind of alert in the edit view that it might not work because of that reason.
Please don't, if there's a regression open an issue and assign it to me.
@dgrammatiko There is not really a regression. But if you create a layout override for com_templates/template in 4.2.x and then update to 4.3 (with this PR included because otherwise you would get an exception by the web asset manager when the default.php of the old override wants to load the obsolete js), the toggle to show the diff view doesn't work and the diff view is not shown when toggling to show that. We could fix that now for that particular case, but in general it can happen again in future when there is a change in the core for the default layout of the template view of com_templates. Therefore it could make sense to show that in that view somehow.
Therefore it could make sense to show that in that view somehow.
It's a amortisation strategy I don't really like, to be honest. Overrides assume complete ownership of the layout + all the shenanigans (css, js, etc). It's more a lack of education or completely false assumptions from the users (ie js/css files are given and cannot go away or change completely their contents). My 2c anyways
@Hackwar already opened a PR for the manual but it is incomplete: joomla/Manual#68
and @brianteeman needs to add the deprecation on the manual too