-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[RFC] First draft of deleted file check #25559
Conversation
build/deleted_file_check.php
Outdated
*/ | ||
|
||
const PHP_TAB = "\t"; | ||
// TODO: Make these directories dynamic or clone from git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You definitely want the --from
and --to
params from my version of this script here, as you're going to want this script to be able to work without someone needing to arbitrarily set up multiple git clones on their own to run it. Then, copy the logic from the build script to run the git archive
command to build your two working directories and do any of the setup steps needed (i.e. NPM and Composer plus cleanup).
The --from
param, you'll probably want to restrict to real version numbers (tags) instead of any valid commit reference like it does now, because you're going to need it to behave differently based on what version you're doing the comparison from (3.x doesn't have build steps, 4.x does). I think it's safe to say this version of the script will only be run when doing a comparison to a commit in the 4.x tree, so that restriction isn't needed for the --to
param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with git archive is that it gives me all my dev deps as well which I then somehow need to unstage after I've run composer install and npm install. I think we actually need to somehow split the build script out a bit because I need some of the common parts like the files/directories to ignore there
9b06915
to
4469947
Compare
Pushed a new tweak up - a horrible version of doing a iterator filter (there must be a better way I can deal with the excluded folders other than the prepending which requires separate filters) - but at least it works. This means com_search and installation directories are excluded and there's a simple mechanism for this in the future. I've also reversed the order of the directories so we delete subdirectories before parent directories |
e48dc24
to
3c2c6e9
Compare
3c2c6e9
to
9d216a2
Compare
Just used this to update the latest script.php in 4.0-dev so will be able to use that as a metric of how stable this script is beyond the requirement to be able to download the two branches (for now i just downloaded and unzipped the latest nightlies of 3.10 and 4.0) |
How to use this script? I tried joomla-cms/build/deleted_file_check.php Line 39 in 9d216a2
|
That's what I did - where the directories are the two nightlies. I haven't changed that line from the working J3 version :/ so is this script working for you there? |
That works, thanks. I haven't used this script before. Should templates be deleted? They can contain custom overrides and assets. |
Currently the intention was yes. Because we’re not shipping bootstrap 2 in j4 so they will be broken by default. However I’m willing to listen on it. If people don’t like that decision it’s easy to add it to the exclusion list |
Lookup the 2.5 to 3.0 scripts. IIRC the admin template was fully uninstalled but the frontend template wasn't. The same should be done for 3.x to 4.0. |
The 2.5 template didn’t break I don’t think because it had no framework under it? Not 100% on that though |
Protostar has its own CSS, it’s not using Bootstrap from the media directory. So that part isn’t busted. The JavaScript integrations probably are though, not much that can be done for that without a compatibility plugin overloading JHtmlBootstrap to restore BS2 (the same as my old BS3 demo plugin). Likewise layouts are busted because of the insistence of the default layouts being designed against the default frontend template and not being agnostic. Still it’s probably be a bad idea to flat out wipe out someone’s template. |
2.5 site and admin templates survived after the update |
@wilsonge For me this is at least a release blocker due to following reason: We want to have updateability from beta-n to beta-(n-1). So latest with beta-2 we need it to be included into our build procedure so the script.php is up to date for each build. If we want to go one step beyond and support updateability between nightly builds beginning with beta-1, then it even could be a beta-blocker. Which of these 2 labels should I set? Feedback very welcome. |
I've added an explicit release blocker - but the root issue it's partly solving is already a release blocker. I don't think it's a beta blocker as we can already use this script - it's just about making it pretty to use |
@wilsonge I think it should run automatically on every nightly build at the end. |
It can not run automatically - its building array's to paste into script.php. unless we made these contents a standalone file - but that's out of scope of this PR. I'm manually putting the results into script.php right now (as was the case in 3.x too) |
dbaac21
to
eee17c8
Compare
Pull Request for some of Issue #16458 .
Summary of Changes
Reworks our deleted file script onto something more rudimentary however that takes into account our npm and composer deps are no longer stored in the repo.
Things left to do
Testing Instructions
This script has been used to generate the existing deleted files list - which are already usable for update - but has had a few modifications since.
Basically in the build create a tmp folder grab a copy of two joomla versions - the 3.x version goes into the a
joomla390
folder and the 4.x intojoomla400
folder (i picked 3.9.13 and 4.0.0 alpha 12) to generate the list in this PR). Then run the script - it should match what's in the com_admin scriptDocumentation Changes Required
n/a