I've addressed #89 but in doing that I've revealed the two of the tests are resulting in invalid composer files so we need to fix that.
Oh and I think we should add a follow-up to implement the ability to unpack multiple or even all the recipes in a root composer.json. Atm the unpack command accepts a single package name.
Okay I think we're 99% done. We just need to add tests for two things:
- The composer lock hash is correct after unpacking a recipe.
- The recipe files are left alone after unpacking.
I like the commitOrRelease name way more than yield. I like specific methods for commit and releaseSavepoint being public.My only concern is that commit not being an alias for commitOrRelease() might be problematic - because ATM DB transaction users don't need to be concerned if something wraps their code in another transaction because the abstraction layer hides this complexity away from them.
alexpott → changed the visibility of the branch 3520750-use-a-transaction to hidden.
Profiling results running time vendor/bin/drush si -y -v standard --account-mail=alex@example.com --account-name=admin --account-pass=PASSWORD --db-url=mysql://USER:PASSWORD@localhost/DB_NAME --locale=de
With MR
________________________________________________________
Executed in 7.29 secs fish external
usr time 3.14 secs 0.21 millis 3.14 secs
sys time 0.94 secs 3.19 millis 0.94 secs
Without MR
________________________________________________________
Executed in 9.45 secs fish external
usr time 3.32 secs 0.25 millis 3.32 secs
sys time 0.93 secs 2.56 millis 0.93 secs
The results are pretty consistent and will benefit sites with more translation files to import.
alexpott → created an issue.
I think we should do 🐛 block_theme_initialize should not create blocks during config sync Needs work which would have the by product of fixing this issue as block_theme_initialize() would no longer trigger during a recipe install.
alexpott → made their first commit to this issue’s fork.
alexpott → created an issue.
alexpott → created an issue.
We are helping people on hosts like Hostinger. They have both composer and composer2 installed but neither will work with automatic updates. Once they update their composer2 you would still need to update the path to their composer2 - currently you need to download the phar and place it somewhere yourself. TBH we could have a follow-up with instructions for how to do that because on a lot of cheap hosts it is going to be necessary.
Discussed with @longwave and @catch given we're never going to upgrade 10.5.x to Twig 4 we don't need to do this one.
wim leers → credited alexpott → .
Okay I've fixed #87 and will still work with a --dry-run ... I'm going to add test coverage for both things and then I think we're ready for another round of review and rtbc :)
So --dry-run works but it reveals another issue. Here's a useful composer.json for testing unpacking.
{
"name": "test/test",
"type": "project",
"require": {
"drupal/core-recipe-unpack": "@dev"
},
"repositories": {
"composer-unpack": {
"type": "path",
"url": "/PATH/TO/YOUR/DRUPAL/MR_CHECKOUT/composer/Plugin/RecipeUnpack",
"options": {
"symlink": true
}
},
"drupal": {
"type": "composer",
"url": "https://packages.drupal.org/8"
}
},
"config": {
"allow-plugins": {
"drupal/core-recipe-unpack": true
}
}
}
Replace /PATH/TO/YOUR/DRUPAL/MR_CHECKOUT with the correct value for your set up...
If you do this then run the following commands on a directory with this in:
- git init
- git add .
- git commit -m "Init commit"
- composer install
- composer require drupal/events
You'll see it is unpacked...
Then do...
- git co .
- composer require drupal/events
You'll see it is not unpacked.
This is because we find recipes being added during the post-package-install event but this is not triggered the second time around because the packages are there. I think we need to do cleverer stuff in post-update-cmd as this is triggered both times. And then we might need to deal with --dry-run :)
Next thing to do is to not do anything when --dry-run is passed into the require command. Working on that.
Committed and pushed 44f56c2c1b8 to 11.x and 371936c49aa to 11.1.x. Thanks!
I think the constructor changes here are in scope because we promote $connection to a property so we are changing the constructor. Imo the ImportStorageTransformer is not in scope because we're not changing them.
The CR is no longer correct as the class is not moving. I do think that the CR should exist - it should detail that by default if you use the development.services.yml you will can warnings when saving configuration that does not comply with schema. Once the CR is updated this can be set back to RTBC. The solution looks elegant and it's great that this emits warnings rather than errors.
alexpott → changed the visibility of the branch 3441434-add-validation-constraints-child-merged to hidden.
I think we can use autoconfiguration here and I have confirmed that the test finds the problem. Nice spot.
lostcarpark → credited alexpott → .
I've got unpacking of require and require-dev recipes working. We've got some decisions about behaviour to decide.
We need to decide how the following situations work...
We have recipeA and recipeB. recipeA depends on recipeB.
What should happen when recipeA is in require-dev and recipeB is in require and you call unpack recipeA?
What should happen when recipeA is in require-dev and recipeB is in require and you call unpack recipeB?
What should happen when recipeA is in require and recipeB is in require-dev and you call unpack recipeA?
What should happen when recipeA is in require and recipeB is in require-dev and you call unpack recipeB?
For more information on this problem space if you do the following on an empty directory:
$ composer init --no-interaction --name test/test --type project
$ composer require --dev symfony/yaml
$ composer require symfony/yaml
The second require will offer to move the dependency. If you answer no then it will not do the include.
Discussed with @longwave and we agreed to put this back to 11.0.x in case we do a security release on 11.0.x.
Committed and pushed 405ac9dad3c to 11.x and 7769a956fbe to 11.1.x and 902e7a8c57a to 11.0.x. Thanks!
Committed and pushed c42f5d9328c to 11.x and 8f3ccf4c4b4 to 11.1.x and f26adfd7445 to 10.5.x and a09b4608957 to 10.4.x. Thanks!
Sure why not - seems more robust.
Committed 9bed1ff and pushed to 11.x. Thanks!
The monthly security window I believe is tomorrow, doss this have any impact on that window?
I don't think so. This is an existing issue that has already been released. If it would be the first release with it in maybe but that's not the case.
I agree with #6 let's just remove the code. We can open an issue to reimplement if we like.
alexpott → created an issue.
I am not going to make changes until we have agreed on a plan forward.
Yeah 100%.
One thing I'm thinking about is rollback... Atm we call rollback on a transaction and that will go back to your previous savepoint or the end the root transaction without a commit - right? So given rollback already does 2 different things... why don't we rename yield to commit() and document that it will commit the root transaction or release the savepoint depending on transaction depth as we already have this behaviour with rollback... what do you think. My concern is the yield is an odd word wrt to databases and transactions.
I've updated the MR to leverage \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait - it'll allow us to remove the module handler property and have less complexity in the constructor.
Added some comments to the MR.
I think we should consider having a commit method for situations where you know that you've created the root transaction. I think we should also consider having a startRootTransaction() method to support this. So you can have very explicit code. Yield is a tricky word to use as it already has quite a bit of meaning in PHP and normally means to yield a value as part of an iteration.
Committed and pushed 5533029b08d to 11.x and 44640216c26 to 11.1.x. Thanks!
I created the MR and took the liberty to sort the other frontend framework managers properly - we sort by maintainers last name with provisional maintainers coming after the full maintainers.
alexpott → made their first commit to this issue’s fork.
longwave → credited alexpott → .
There are quite a few usages in in contrib - see http://codcontrib.hank.vps-private.net/search?text=-%3EgetValue%28%27con...
I discussed with @longwave - the thing that's tricky here is that we are only removing this because it is dead code but the value is still being used by contrib and these modules will break silently because the deprecation we're adding here is only for the method. The current implementation will break modules using $form_state->getValue('confirm') immediately. Obviously form arrays are not part of the BC promise - see
https://www.drupal.org/about/core/policies/core-change-policies/frontend... →
- but I think we can/should do better. I think we should consider adding a #deprecated key to the form arrays so we can issue a deprecation when calling $form_state->getValue() for a specific key. I think doing this would allow us to remove old stuff like this and get modules to update without breaking stuff.
I think we should postpone this issue on adding this capability to the form system. Going to set this issue to needs work to open the issue to do this and then we can postpone this on that issue.
Forgot to say - great issue summary - could be used to form the basis of the CR.
We're going to need a change record here here that we should keep updated as each follow-up issue lands. Going to leave at RTBC to get committer and framework manager reviews
We need a change record to detail the addition of StateInterface::getValuesSetDuringRequest() -
https://www.drupal.org/node/add/changenotice?field_project=3060&field_is... →
Committed and pushed 8c33fb4f4ad to 11.x and bb439263bd2 to 11.1.x. Thanks!
Backported to 11.1.x
Yeah using the command line would be nice - but there's 🐛 The command line runner has no way to pass in the URL of the site Active which concerns me.
I've tested this on a large site and tested removing quite a few links. It makes the batch processing much much nicer.
The big issue left is the new constraint - left a big comment on the MR.
alexpott → created an issue.
This is working nicely.
We need to move the hook implementation to the correct new OOP place... \Drupal\system\Hook\SystemHooks
The reason this happens is that when we return from the node preview getRevisionId() returns NULL because we dealing with an entity that will become the next revision.
Nice find. Thanks Thunder tests!
alexpott → made their first commit to this issue’s fork.
I;ve looked for other links to stackoverflow in our code base. There are a few the others seem to involve much less code but I guess need to be reviewed as well.
Yep we need to re-write this code.
I've tried to find out in the issue comments but I can't see where it is decided but I'm really surprised by the schema changing when the fast 404 is enabled or disabled. This is not a regular thing. Normally you can disable something and leave the other config in place. I'm not sure why the complexity has been introduced. Setting back to needs review to get an answer.
Some review comments to address. Two I would have fixed on commit. One which needs a little bit of thought hence back to needs work.
We need to update the issue summary. I think we need to consider the correctness of the code and what it should be doing. Also the issue summary is incorrect - there's no PHP notice about an undefined variable anymore.
I've tried to work out why but unfortunately this code was added to views before the great CVS migration so it is seriously old code.
For me the big question here is why do we have this code at all. My guess is that if the argument is numeric then this is some very basic validation we can do. We should have a comment that indicates this in the None plugin.
I think this is ready to go.
alexpott → created an issue.
@marcosano has pointed out that entity embed is a problem. Entity embed possibly will add links to other entities and we wouldn't want these usages registered as part of the entity with text field... so yeah #4 is wrong-headed.
That said I think it points to the fact that we could reorganise the plugin relationships so we only have to create the dom for each text field once.
I've been thinking about this issue and it makes me wonder if we should be maintaining:
- \Drupal\entity_usage\Plugin\EntityUsage\Track\EntityEmbed
- \Drupal\entity_usage\Plugin\EntityUsage\Track\HtmlLink
- \Drupal\entity_usage\Plugin\EntityUsage\Track\MediaEmbed
- \Drupal\entity_usage\Plugin\EntityUsage\Track\LinkIt
- \Drupal\entity_usage\Plugin\EntityUsage\Track\CkeditorImage
I think we should have a single text field plugin that uses the processed text (and summary if available) to determine usage. The plugin should support entity usage discovery from both the URL and the data-entity-uuid and data-entity-type attributes.
This approach would have the following advantages:
- No double reporting from LinkIt/ EntityEmbed / CkEditorImage / MediaEmbed - these plugins are all essentially the same - they use the attributes to determine if there is a link
- A single plugin working through all the links is more performant and less code to maintain
- Support for any esoteric filter that adds links in a way not covered by the existing plugins
Now to add test coverage...
@phenaproxima I added test coverage of an existing dependency. I've also got test coverage of moving a dep from require-dev to require and not moving a dep from require to require-dev :)
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
If we changing the 10.3 database dumps because of invalid schema this points to us needing an update function. I think we should not be changing the dumps.
Also I think we should be adding a new constraint that works like callback but uses the class resolver service to my more flexible.
Committed 2778882 and pushed to 11.x. Thanks!
alexpott → created an issue.
alexpott → created an issue.
I've tested this solution with easy breadcrumb as described by 🐛 Adding url.path to breadcrumb cache context results in every 404 page getting a different dynamic page cache entry Active and it works fantastically. This is a fantastic change which will improve the cacheability of any block that is using the response status condition.