Time works for me and the MR looks good!
This is a great idea!
The edited paragraph might need some thought, though, as the topic of development tools is less likely to be related to the core gates.
Okay, I believe this is easy for review.
rachel_norfolk → created an issue.
Made an RC1 release
well, I think it is time to make a release...
Looks good to me, too.
Thanks everyone!
rachel_norfolk → made their first commit to this issue’s fork.
I agree that we need some tests. I also agree that creating those tests shouldn’t hold up an initial release of the project, it can come later.
updating list...
Awesome work, everyone!
I suppose having tests that fail against 3.x now but start passing once that other change request is merged will be a rather appropriate test ;-)
@nicholass - I was going on the message in comment #6. There’s certainly a belief that the issue is fixed in @dev, but in a different way.
Purely pressing the Create Merge Request button so it is easier to see what we have and it can be reviewed...
updating status...
(annoyingly, this only happens on save atm)
updating status...
Heh - I’m slightly amused that it took me longer to remember how to make a test site with a base path than it did to actually test the proposed change!
With the change applied, the test site correctly takes into account the base path. It also continues to work when there isn’t a base path.
There is an update function to update aliases that might already be stored, which is also nice.
I think this is a good one. RTBC
Setting to maintainer needs more info as that’s kinda what we are looking for.
The project homepage does say that the module only works on content entities that extend Drupal\Core\Entity\ContentEntityInterface so this might be out of scope at this time.
just adding [duplicate] to title so I don’t keep looking in here for things to do...
Added comment to say we don’t need to drop support for ^8.8 at this stage - unless there is something I’ve missed?
rachel_norfolk → changed the visibility of the branch project-update-bot-only to hidden.
feels like needs work...
Nice work, everyone!
Looks good to me, too!
Just updating the assignment. In general, we tend to just have people write a comment saying they are working on something and for how long. That way, it avoids it getting left assigned (and I used to be massively guilty of this!)
rachel_norfolk → created an issue.
wanna have a go at it?
Added the wording suggested above - it does help!
This is a great piece of work! I’m beginning to worry I’ve pitched some training I have organised for you a bit low! :-D
I’ve added a couple of comments that I think will make it more maintainable.
I like the direction, though.
I was just coming here to say how to test and you beat me to it!!
Thanks for the review.
Nice one!
I also slightly changed the version requirement to at least 10.1 as Change Record says that is required for that method of calling the hook in a class.
I think you still need to press the button in Gitlab to tell it to create the Merge Request.
Before you do, though, one thing worth bearing in mind is that, whilst inside a .module t() is acceptable, once in a Hook class, we really should use the StringTranslationTrait to add in $this->t() and use that.
You can see an example in core/modules/block/src/Hook/BlockHooks.php
Also, I’m still getting used to reading these new hook styles so I might have missed something!
Needs a review
rachel_norfolk → created an issue.
Okay, you have me intrigued as to what the performance concerns might be.
There’s currently a hook_form_alter that checks to see whether the form class has a particular method. See:
$form_object = $form_state->getFormObject();
if (method_exists($form_object, 'getEditableConfigNames')) {
//Add a message.
}
Plus checking for a couple of other specific form classes that we know would alter config but work slightly differently.
Do you think inspecting the class would be intensive?
rachel_norfolk → created an issue.
rachel_norfolk → created an issue.
rachel_norfolk → created an issue.
Really nice work all - you are now Drupal contributors!!
I’ve madde a couple of tiny modifications but I’m loving what you have written!
I’ve (just) added the Config Warning → contribution module but I definitely see this as “backfill” for something that should really be in core.
rachel_norfolk → created an issue.
I’ve started to add content to the docs so it helps people know how to add things and shared with mentoring-leads.
Enjoyed the meeting tonight!
- There are messages displayed if config is overriden in the settings.php file (so that site builders don't get confused why the config change in the GUI doesn't have an effect). That should work together somehow, and look consistent.
-- yes, I would suggest the code sits alongside that, too. - If something is in config ignore, then it also shouldn't show up - or show up with a different message?
-- very good point! Should be a reasonably easy check to make, I expect - This shouldn't show up on a development site because it would drive site builder crazy. May a toggle to turn it off, that can be overridden in a local settings.php file?
-- yeah, I was thinking we could have a option in the development section of admin that is turned off by default - a site owner only turns it on for prod sites, if they want. - The wording of the message: This is also relevant for projects that don't have a dev team - as long as there is a separate copy of the site for development and exporting config. Something like "This configuration should better be changed in the development environment and then exported. Otherwise they will be reverted back during the next config import."
- And maybe... and option for an extra field for whom to contact if this really needs changing on prod to ensure it doesn't get lost?
-- yeah - another option in the development area, I would suggest, though I guess the whole text could be overidden just like any text can be overridden in the site.
The code above actually works, Tony. It inspects what base classes the form is implementing and can make a prediction of whether it will affect running config from that. This means it even works on contrib modules!
mradcliffe → credited rachel_norfolk → .
updating vienna names
mradcliffe → credited rachel_norfolk → .
rachel_norfolk → created an issue.
Note: BlockListBuilder extends ConfigEntityListBuilder and that ’s probably worth looking out for, rather than the special case of blocks.
Playing around with this a little and the following working “pseudo code” (i.e. please don’t actually use this as is - it is just to test ideas) gives the idea of what I’m thinking:
function xx_form_alter(&$form, FormStateInterface $form_state, $form_id) :void {
// Get out of here if it is not an admin form.
if (!\Drupal::service('router.admin_context')->isAdminRoute()) {
return;
}
// We need an actual object rather than just a jumble of array items.
$form_object = $form_state->getFormObject();
// Assume forms have no config to begin with.
$has_config = FALSE;
// Does the getEditableConfigNames method exist on the form?
if (method_exists($form_object, 'getEditableConfigNames')) {
$has_config = TRUE;
}
// Are we editing an entity that has config form for it's setup?
if ($form_object instanceof EntityForm) {
$entity = $form_object->getEntity();
if ($entity instanceof ConfigEntityInterface && !$entity->isNew()) {
$has_config = TRUE;
}
}
// Permisions being permissions.
if ($form_object instanceof UserPermissionsForm) {
$has_config = TRUE;
}
// Blocks being blocks.
if ($form_object instanceof BlockListBuilder) {
$has_config = TRUE;
}
// Okay, warn people.
if ($has_config) {
\Drupal::service('messenger')->addWarning(t("This form is likely to alter the running configuration of the site and, therefore, changes should probably go through the development quality processes."));
}
}
Adding an option for the warning to include a custom message.
Adding link to Config Override Warn as inspiration
rachel_norfolk → created an issue.
changes needed
- add step to set status of new cloned issue to Active
rachel_norfolk → created an issue.
Thank you all!
I tell you what, lostcarpark, let me run this meeting to test the script above, then you can test any changes we make next month?
adding contributors
Following the notes for the meeting next week as testing...
just editing slightly as I think we said it was me looking at GitLag, not Chris?
rachel_norfolk → created an issue.
adding that spreadsheet structure to get us started... https://docs.google.com/spreadsheets/d/1WdwP4fFr2k2ADOTWB59w1evjdf6djAdB...
rachel_norfolk → created an issue.
Just a note that this change would be an absolute gift to those who like to game the credit system
I do love the seemingly random way component is set as we move issues between projects ;-)
As we are happy as a team to progress this, it seems time to move into the core project so someone can make an MR.
mradcliffe → credited rachel_norfolk → .
Just formatting the tag correctly so it gets counted along with all the others.
I would be delighted to add James to our list. He is bringing exactly what makes mentoring successful.
chrisdarke → credited rachel_norfolk → .
Who authored the node? I can’t remember.
mradcliffe → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
mradcliffe → credited rachel_norfolk → .
Hi - did you mean to post this here? This is not a support issue queue.
I suggest you ask the question on the Drupal Slack Drupal.org/slack
I hope you don’t mind me saying, sarveetsingh, but that is a really well written test comment. Contains everything it should. Nice work!
mradcliffe → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
liampower → credited rachel_norfolk → .
Adding credit for Ángela Saldaña Contreras who made the logo actually work for us!!!
That’s good news, sali38, and much appreciated!
For the record, there seem to be a number of other related projects with the same restriction claimed.
rachel_norfolk → created an issue.
They will quite possibly have a Bash CLI. Especially if they are a new user, though, it may not be clear at all that they need to use it.