[#39413] - [4.3] Handle missing file in updateAsset method of the versioning js when building packages
- Closed
- 16 Dec 2022
- Medium
- Build: 4.3-dev
- # 39413
- Diff
- richard67:4.3-dev-fix-versioning-js-for-missing-assets
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
After PR #39374 has been merged, the package build e.g. with php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2 fails at the versioning step for the assets because PR #39374 has removed some JS but hasn't removed it from file build/media_source/com_users/joomla.asset.json.
It was partly also my fault. I should have noticed this when reviewing that PR, but well, nobody's perfect.
But when testing the PR it was not obvious because package building doesn't belong to our usual testing instructions and also not to the automated CI tests, and the package build which drone runs for building the testing packages seem not to include that versioning step because it hasn't failed for that PR. Everything was green.
Pinging @Hackwar just for information.
This PR here fixes the issue by changing the versioning js so that it can handle missing files for assets referred to in a joomla.asset.json file. Thanks to @dgrammatiko for the code snippet.
Removing the obsolete assets from the joomla.asset.json would cause exceptions by the web asset manager when a 3rd party template would still try to use that asset. That's why it can't be done this way. Thanks @SharkyKZ for pointing to that in my previous PR #39408 for this issue. I hope you see that giving comments with additional info to a thumb down helps. It might be more time consuming for you than just giving an uncommented thumb down, but the result for the CMS is definitely better, and I assume you want to help the CMS, or not?
Testing Instructions
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 today.
- 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/39413/head:test-pr-39413and thengit checkout test-pr-39413. - Repeat step 2.
Result: See section "Expected result AFTER applying this Pull Request" below.
Actual result BEFORE applying this Pull Request
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
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
joomla-cms-bot
- | Category | ⇒ | JavaScript Repository |
| Status | New | ⇒ | Pending |
| Labels |
Added:
Release Blocker
PR-4.3-dev
|
||
My test should still count, right?
@dgrammatiko Why should it not? Ah you mean because you suggested the code? Well, I think that should be ok. Of course I've tested it myself, too.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39413.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39413.
Ignoring missing files in build script is just asking for trouble.
Ignoring missing files in build script is just asking for trouble.
I think we can tolerate it in this specific context. Let me explain:
The script is reading recursively all the asset.json files, gets all the js/css paths resolves them and then reads all the files to create a hash for each one that it would then append to each asset entry in the respective json. Pretty basic staff. Even if all of these fail and nothing gets into the asset.json file Joomla still has the classic versioning from the DB MediaVersion (which actually now is a stain file in the cache folder). So I think it's ok (especially if someone considers that removal of asset.json entries should be forbidden for the B/C break you mentioned in the other PR)
The build should file if files declared in asset registry are missing. The file in question shouldn't have been removed in the first place.
| Labels |
Added:
?
|
||
The build should file if files declared in asset registry are missing. The file in question shouldn't have been removed in the first place.
@SharkyKZ So we can never remove any js or css which once was in any joomla-assets.json file? Have to keep that forever? I remember some comment from you in PR #39327 : "Then it's time to archive the repository because no further changes can be made.". It seems the same comment now would apply on what you write here. Maybe you have some kind of a multiple personality disorder? Just asking.
Well, we can add back the deleted js files from PR #39374 , maybe add some comment to them at the top that they are deprecated, and if that is possible also add a comment to the joomla.assets.json about that.
But then I would like to know: Do we have to do the same with the js and the changes in a joomla.asset.json from PR #38823 and possibly others?
See https://github.com/joomla/joomla-cms/pull/38823/files#diff-67f83219b7102940384c0fa4377e85b81a34dade675b25ddef641a5169fe53b8 and
https://github.com/joomla/joomla-cms/pull/38823/files#diff-e1d246ef13cbc1c9d34bf89f4fc99e72bd7868b105693cb09745928510fee552
The build should file if files declared in asset registry are missing. The file in question shouldn't have been removed in the first place.
The files are RIGHTFULLY removed from the NEW VERSIONS! Probably the project should revisit the removal of static assets on updates. So this PR is still on the right track, existing instances will have the correct asset.json (the version will be picked from the DB, that's fine) and the files already in the local filesystem. The update WILL NOT harm them. New installations do not need the JS files because they don't use them. Are you ok with this @SharkyKZ ?
Probably the project should revisit the removal of static assets on updates.
@dgrammatiko Should I change my PR #39401 and delete these lines https://github.com/joomla/joomla-cms/pull/39401/files#diff-db7eb77540ff419bd7e6557d2779f4cf1e31158c20cb4b63dbbe8d6c426385adR6486-R6491 ?
@richard67 probably that and the other cases you mentioned above as well, but I guess you should better get some more consensus here from the maintainers...
probably that and the other cases you mentioned above as well, but I guess you should better get some more consensus here from the maintainers...
@dgrammatiko I've asked for opinions but did not get anything clear up to now. Some don't see a problem, some haven't answered.
| Status | Ready to Commit | ⇒ | Pending |
Back to pending due to recent change.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39413.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39413.
Neither assets, nor their associated files should be removed or changed in a way that breaks their usage. Templates, like any other extensions, should not break on every other patch release. That's what major versions are for. But that's just me. It's up to PD to decide if they want to keep screwing Joomla users and developers over and over again.
The particular file in question is not that important, at least the form can be submitted without it. But what practical gain is there to removing it at this point? More importantly, this sets a bad precedent going forward. There should be more consideration for template developers.
I think we can tolerate it in this specific context. Let me explain:
Nothing to do with versioning. It's to do with not allowing missing files to slip through the cracks. Builds failing at this stage would notify about assets and their registry files being out of sync.
@SharkyKZ I value your opinion. I'd like to know your opinion on this other PR #38823 , which removed JS and their assets from joomla.assets.json of com_templates (for the diff view when comparing overrides). Is the importance of that similar to the one discussed here? Or is that more critical? I think it's not, but I might be wrong of course.
It's up to PD to decide if they want to keep screwing Joomla users and developers over and over again.
There should be more consideration for template developers.
@SharkyKZ Regarding your comment, I think it's a valid point, and I have asked the other maintainers for discussion and decision. I think you know that I am not the guy who can and wants to decide such things alone. As 4.3 is still in alpha phase I think there is still time to revert the one or other thing (removal of js files from sources and from the json and deleting the js files on updates) and possibly have a kind of list of deprecated assets and log their use somewhere.
It's the same thing. Any existing layout override is going to break because the asset is removed. Also that SQL query is awful and totally unnecessary.
It's the same thing. Any existing layout override is going to break because the asset is removed. Also that SQL query is awful and totally unnecessary.
prove it
Also that SQL query is awful and totally unnecessary.
I was not happy about that query either. It's not really necessary when the right defaults are used when reading the parameters in PHP. It is safe as long as we don't have nested json objects in that params column, so I decided to live with it.
I did not have the courage to request it to be removed because I was not keen on a discussion.
Maybe I should not be a maintainer.
So, maintainers do not want to ship dead code for security and housekeeping reasons. I think everyone can respect their views on this except @SharkyKZ who believes that this constitutes a B/C break for the overrides. Hmm, so what's an (layout) override? It's a file that takes over the default one that the CMS shipped and it assumes that the creator also takes complete ownership of not only the HTML that's inside that file but also all the static assets (CSS/JS/images/etc) that the view might require to properly deliver the experience. So, I think if someone just create a layout override, tweak couple lines of HTML and still depend on JS from the original extension (on the previous PR I mentioned that only media/system and media/vendor should perceived as public API, eg you can trust they won't break) they're on a path for failure, sooner or later. Maybe the project should set the expectations right with a very clear definition of the term override...
and what is the point of the check override functionality - we might as well delete all of that
I wonder if there are so many overrides in the wild for backend templates...
and what is the point of the check override functionality - we might as well delete all of that
It's funny you should mention this because that's exactly what your changes would break, Users couldn't even see the list of overrides.
We have discussed in the CMS Maintainers team. I will replace this PR here by a new one on Friday because I don't have the time today and tomorrow. As it looks now (still collecting opinions), the removed JS files will be added back so that the change from this PR here will not be needed anymore. Leaving it open because not much time now to create a new issue instead.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-12-16 13:15:38 |
| Closed_By | ⇒ | richard67 | |
| Labels |
Added:
?
?
Removed: ? |
||
@SharkyKZ @dgrammatiko Please check and if possible test #39431 . Thanks in advance.
I have tested this item✅ successfully on 539a4f9
My test should still count, right?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39413.