We finally got around to doing a bit more testing on this.
We also see that editing then shows the URL including the alias, and that is weird, but I don't think there's anything special about multilingual here, it also does that on the default language.
What we can no longer reproduce, with the formatter change from the other issue, is that the link in the frontend is broken. We saved the translation with the URL including language prefix, and then changed the alias of the target node and that alias was immediately updated and displayed correctly in the linkit field.
I'd suggest to close this, Ithink there are other option issues around inconsistent display with/without alias when adding/editing content.
You made the test method static. That's wrong.
Merged.
✨ Suggestions for embedded entities Needs review should already cover that.
Yes, this conflicts badly with another feature we added.
Makes sense, but instead of the if, it's the isBaseField() ternary inside that we should check for it being a FieldStorageConfig instead.
FWIW, I'm not sure why we even do that separation, I think we should instead just always combine entity_type_id and name together, it's also possible that there are multiple base fields with the same name.
there's also another isBaseField() check that might also have a problem with this, because views_entity_field_label() only deals with FieldConfigInterface, so I think we should do the same and check for that interface there.
This is a relatively complex workaround for an underlying issue. If you get those locks on cache tables consider using a different cache backend like redis, you might run into exceptions like that also when regular editors save content.
I don't get the setting. It's not an exception anymore, just a warning, and I think it's worth to always do that, don't see the benefit of hiding the fact that some provided translations couldn't be saved.
The second part of my review in #22 wasn't applied and tests weren't added either as I asked in back in #15.
Reviewed.
\Drupal\tmgmt_content\DefaultFieldProcessor::shouldTranslateProperty should prevent this. Could you extend \Drupal\Tests\tmgmt_content\Kernel\ContentEntitySourceUnitTest and add a list field and and assert the resulting extracted data structures?
berdir → made their first commit to this issue’s fork.
Please provide contributions in the future as a merge request.
Please convert this into a merge request, too old to commit without recent test results.
Thanks for the MR.
Some thoughts.
a) Since this is in tmgmt_content, the hook should be namespaced tmgmt_content, so it doesn't conflict if we'd add the same to another source.
b) Should $data be passed to the hook? you might want to use the information in there?
Committed.
Committed.
These queries aren't going to fix themself on Postgres/sqlite, this needs to be updated to a merge request.
Committed patch from 🐛 Paragraphs translations aren't always updated Fixed
Committed.
The reason it's not null is because you have an entity key, and the current logic is that those fields get the not null addition. Pretty sure there are issues about that. As always, change is hard.
Node does the fall back to 0 if the owner is not valid, but it's custom to node.
Pushed a number of improvements, job queue is now properly cleaned up, jobs are submitted and the user is redirected back, as with the batch.
> One other thing @Berdir also mentioned is that there could be different thresholds per source there or even add configuration for antibot/honeypot triggers.
not per source automatically but only configurable. The idea is that the /antibot and also honeypot are much more likely to be hit accidentally by real users than some wordpress thing. There are for example bugs in antibot in case of empty multistep forms where you can click without a mouse movement. So there is an argument to have higher thresholds for that.
Also, the route based approach is a workaround due to antibot not having a hook/event.
It is a duplicate right now, but maybe it shouldn't be? This does the lookup only by uuid, but we also store the revision id. the revision uuid field never landed in core, so it's difficult to replicate what the regular ER field does for revisions.
I think an ERR field with a default would be extremely rare, no idea what the use case for that would be, so we can maybe just ignore that.
#24 tells me that people don't don't that you can hide both promote and sticky through the manage form display page, since 8.0. No extra modules or custom code required.
Removing/moving those fields sounds like a very scary upgrade path issue to me. We know that people don't read and they also don't do backups, so upgrading means that we will drop their data.
What if we just do a very basic change for now, and that's to have those fields hidden by default. If you want to use them, make them visible, otherwise you likely won't notice that they still exist. That's as simple as removing two method calls in the field definitions and has very limited risk.
Reviewed.
drunken monkey → credited berdir → .
Reviewed and verified the test fails are due to caching.
The other thing is caching. It should be consistent in a test, but I'd expect that multiple requests to the same URL would end up in page cache. I'd suggest using different unique URL's.
drupalGet() doesn't expect have a leading /, try without that. Tests there run in a subfolder, that might end up outside of Drupal.
Since this is relying on existing settings, I'm a bit concerned about both cache/invalidation issues and just in general these things suddenly showing up when previously they did not. Having some automated tests that visit/use these links with different users would definitely increase the chances of this being committed.
flag is D10 and D11 compatible, these have already been fixed I think.
Adding new parameters to an interface, even when optional, is an API change. The project is beta, but it's been beta forever, so conflicted about adding this.
One option is to not add it to the interface and just the implementations, that is allowed.
Other thoughts:
* Also changes routes to require additional parameters.
* Not sure about the warning log message. That could result in *many* warnings on a busy site. Just the trigger error might be sufficient?
* theme suggestions based on the flaggable id seems overkill. I understand it's already there, but I don't think we need to add even more variations.
Just committed 🐛 Error when deleting orphaned flags Fixed instead.
Doesn't hurt I guess, although something went wrong if you end up in this situation and you might get other weird problems.
Preparing a merge request helps to get those flags committed, I don't have to do it manually then.
I'm not entirely sold on the whole actual flag idea. Not a native speaker, but for me, flag is more about a marker than an actual flag. I don't know how to visualize that though, and the old one was also an SVG. At least it's easy to identify and remember, even though it might not actually help a new user identify what this project does.
Merged.
Thanks!
@berdir It is catching up the submodules but I guess those modules which is specifically mentioned in the static $modules = []; and any of those modules in case missing from info.yml or composer.json require/require-dev section aren't covered with before_script.
No, it's not catching subfolders. Not without the globstar stuff being enabled: https://www.linuxjournal.com/content/globstar-new-bash-globbing-option. We need to use find.
Neither $modules definitions nor require-dev has anything to do with this. require-dev would fail on the composer step, $modules would complain about missing modules. It specifically complains about non-compatible, which is what the sed is trying to fix but doesn't find all files to replace.
Unsure why the undefined property error happens, looks like the query somehow doesn't return what we expect. could be a weird side effect of running this on the system database on CI, so you likely can't reproduce that locally.
> A TOTAL OF 1557 SNIFF VIOLATIONS WERE FOUND IN 94 SOURCES
That's too much to combine with cspell, it's most likely also too much to do in a single issue on its own.
There's a php deprecation about an undefined property, that's a PHP 8.3 thing that needs to be fixed too. Plus the comment on commerce info files. not sure what the disk error is about, will need to be investigated.
This gets a -1 from me, at least in its current form. We use __get() in all of our projects as the primary method for retrieving entity fields because of how brief it is, so this would be a very time consuming change for us to have to deal with.
I'm aware that the impact of this is massive. This isn't going to happen any time soon. We're many layers of issues and new APIs away from achieving that. See related issues, we need to deal with original and also other stuff that's thrown on entities at the moment. And there's no movement at the moment on those issues.
There are two aspects on your example.
The first is the problem that get() throws an exception on unknown fields. That used to make sense, but since ?-> exists, that indeed seems awkward, and I've been thinking about removing that exception for a while now. It would be great if you could start an issue to propose that. The good thing is that our API doesn't enforce the return type, so we can maybe change the behavior. The only problem I see is someone is relying on the exception and is catching that, then it would result in a php warning or something instead. like this:
<?php
try {
echo $entity->get('field')->value;
}
catch (\Exception $e) {
}
Lets discuss that in that issue. One hack that we've done before on behavior changes is to allow to control it with Settings, so you can opt in to the new behavior and will be the default in D12.
The second part of your comparison is exactly *why* I want to deprecate those methods. Your example implicitly assumes that you have a content entity with magic __get(), if not, that logic would result in PHP warnings. And no, $entity?->field?->value doesn't protect against that, only against $entity being null. Your code that's working with entities is going to be in a function, be it a hook or an API, and you can already type hint there. If your method is only typed to EntityInterface then you'd already get that warning if you receive a non-content entity.
Started with an initial test of replacing the importer completely. Tests are failing, one case seems to be that it doesn't like non-existing fields.
A more feasible approach is likely to wrap it inside since the API has changed, that should make the transition easier, and we could then look into deprecating our API.
Thanks, one more review.
Yes, 10.2 is fine.
.drush.inc is now gone, but I forgot to update the cspell.
I also saw where the simplesitemap thing is coming from, see ✨ Drush comand to export all items from entity with defined criteria Needs review , that's bogus and should be fixed as done there.
This methods don't exist, you're using a patch.
There is a large overlap between this issue and ✨ Drush comand to export all items from entity with defined criteria Needs review . This is easier to use for bundles and supports multiple values, but doesn't support common use cases like all links of a given menu. I recommend to join forces over there and address my review and merge in the useful bits from this like multiple values.
Another review. I'm quite conflicted between this and ✨ Add `drush dcer --bundles` option to limit export Needs review . It doesn't make sense to add both, and while this is more flexible, it doesn't support some things like multiple bundles that the other issue supports and it's more complex to use.
I think the menu use case is a good example why just bundles aren't sufficient, maybe we can expand it to also support comma separated values and add that as an IN condition to the entity query and take that from the other issue.
To be honest, not entirely sold on the new method. It needs a lot of work on docs, API changes on ExporterInterface and now that the old drush changes are removed, is really only used once. Instead of addressing all the review points, it might be easier to just inline the entity query into the drush command again and add the condition there. It doesn't even need a loop then since it really only supports a single condition.
@delacosta456: I recommend reading the built-in documentation that the drush command automatically provides, that would tell you that the property name and value are no longer arguments but options that you need to pass with --property-name and --property-value.
removing the revision affected field seems sensible, that is calculated on save anyway. Should be updated to a merge request to verify that tests still pass.
Merge request wasn't updated with recent patches, and I disagree with the API change to EntityInterface. default_content is about content entities. There's no need to expand that.
Tests seem to be failing.
This depends on another issue and can't be RTBC yet.
And yes, as discussed, it looks like this might be a workaround for an issue in LB itself, but not using that
+++ b/src/Normalizer/ContentEntityNormalizer.php
@@ -178,18 +179,45 @@ class ContentEntityNormalizer implements ContentEntityNormalizerInterface {
+ return $entity;
+ }
+ // Adding support for Entity Reference Revisions entities. This is used by
+ // popular contrib modules such as Paragraphs and Commerce.
+ // @todo Drop the method_exists() call once
+ // https://www.drupal.org/project/entity_reference_revisions/issues/3336752
+ // is fixed.
+ elseif ($entity instanceof EntityNeedsSaveInterface && method_exists($entity, 'setNeedsSave')) {
+ $entity->setNeedsSave(TRUE);
+ }
the issue is now committed, would conflict if an older version is used, one option would be to add a conflict to composer.json, not sure if that's worth it.
The created merge request is for #197, not #196. Please create the correct MR so that tests on CI can run with this.
Looks like a version of this is already in the core importer API.
To early to require 10.3.
A bit pointless now I feel, per #3 this only affects deprecated hal+json import, and I hope nobody is still using that at this point.
Merged, after rebasing to remove the .drush.inc changes (which is now removed).
Never used the module based exports myself, doing that with recipes in the future might be more useful, we'll see.
Merged.
Confirmed to be a duplicate in slack.
Just merged 🐛 README.md in 2.0.x branch still talks about .json files, confusing users Fixed , which should cover most if not all of this issue.
Merged.
Merged, but instead of ignoring drush.inc, I'd really like to drop that, it's dead code, drush 8 is long dead for D8+.
Issues/merge requests to remove that file and update ignore list would be welcome.
I'm not really willing to drop last minor testing, and even less so to to create a major new release because of a single new optional config key.
I understand what recipe API is trying to do, but this feels like a adding quite some burden on maintainers just because core decides to add a new feature. What's redirect supposed to do when views adds another such key in the next minor? and another in the next? Release new minor/major versions every time that then need to be maintained?
Maybe config schema should include a default value system, and missing values that match the default could be ignored somehow? Just thinking out loud.
Plus the usual default views key that needs to be removed that causes most test fails right now.
default_content doesn't delete anything, and even if it would support that ( there are some uninstall issues in the queue), that's not the focus.
What that probably means is that the reference is there, but not tracked as a dependency, so default_content doesn't know that it needs to create the node before the comments.
> When using Drush 12, the commands can not be run anymore.
The commands work fine for me even on drush 13 on drupal 11, so I'm not sure where this comes from.
It makes sense to update it, but per #7, should then actually use autowire.
As it is, this is pretty disruptive to a lot of other issues and merge requests that extend drush commands and I don't plan on merging this soon unless it becomes necessary.
Merged, thanks.
Adding a new parameter to SubstitutionInterface::getUrl() breaks BC.
7.x is technically only in alpha, but it's still challenging. There's an open issue in media_entity_download where people then get a fatal error when using this patch.
Doing this through the filter means this possibility is not available to link fields or it would need to be implemented separately there.
We needed this too in a project, and implemented it with a custom substitution plugin instead.
Merged.
entity.path_alias.collection is in core, not in pathauto.
pathauto patterns are config entities, so views is not an option. Not sure if it really makes sense to replicate the full functionality in a dashboard, because it needs quite a bit of UI space to show the patterns, what they apply to and so on. Some kind of summary/suggestions block might make more sense? (like, X patterns, things without pattern found, but that likely requires some hardcoded assumptions).
Moving my discussion from https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/242... here. I find it hard to keep these two issues separate, and IMHO this should be done first as it would simplify the other one and make it less weird.
Taking a step back, the goal to me: as a contrib maintainer (who maintains many projects) I'd like to have to make as few changes to my .gitlab-ci.yml and scheduled pipelines as possible, while also avoiding unnecessary/duplicate jobs to save money/energy.
I'm struggling a bit with the practical difference of an empty check vs. equals check, especially when we switch to a main branch in the future instead of the current 11.x branch. As there will always be a main branch then, and whether or not it's the next minor or next major just depend on when/where we are in the development schedule. So I don't see how a comparison check would work exactly.
An explicit flag for next major sounds interesting ($_NEXT_MAJOR_NOT_NECESSARY_NOW or whatever we want to call it), as it makes it explicit and gives the template full control over it. but it IMHO also means laying about the actual value of the variable.
Making it more specific again. What #3463894: Update templates so 11.0 is the default/current branch → is currently doing is this:
CORE_SECURITY_PREVIOUS_MINOR: "10.2.2" (unchanged)
CORE_MAJOR_DEVELOPMENT: "11.x-dev" (unchanged)
# Development branch for patch level development/bugfixes.
- CORE_SUPPORTED: "10.3.x-dev"
+ CORE_SUPPORTED: "11.0.x-dev"
# Once the current minor-dev branch enters a beta phase, this is the dev branch for the *next* minor version.
- CORE_NEXT_MINOR: "10.4.x-dev"
+ CORE_NEXT_MINOR: "11.0.x-dev"
# The most recent, stable previous major version of Drupal.
- CORE_PREVIOUS_STABLE: "9.5.11"
+ CORE_PREVIOUS_STABLE: "10.3.2"
# The most recent security release in the previous major version of Drupal.
- CORE_PREVIOUS_SECURITY: "9.5.11"
+ CORE_PREVIOUS_SECURITY: "10.3.0"
* current and next minor is identical, so we run the same thing twice.
* previous stable is actually more recent than previous minor because that's the situation with the last security release. If we'd change that to last minor branch then that would also be 11.0.x-dev then?
* CORE_PREVIOUS_SECURITY = 10.3.0 seems wrong, that was not a security release. that should also be 10.2.2? don't know if anything uses that though.
If we go with checking and supporting empty values for those variables, I would set it up like this:
CORE_SUPPORTED: "11.0.x-dev"
CORE_SECURITY_PREVIOUS_MINOR: ""
CORE_MAJOR_DEVELOPMENT: ""
CORE_NEXT_MINOR: "11.x-dev"
CORE_PREVIOUS_STABLE: "10.3.2"
That means disabled previous minor, disabled next major. next minor runs on 11.x-dev. When 11.1.x alpha releases start and 11.1.x-dev as a branch is created, we can switch that to that. I would keep major development still empty. Once 11.1.0 is released, it would become:
CORE_SUPPORTED: "11.1.x-dev"
CORE_SECURITY_PREVIOUS_MINOR: "11.0.X" (assuming that there will be a security release before 11.1)
CORE_MAJOR_DEVELOPMENT: ""
CORE_NEXT_MINOR: "11.x-dev"
CORE_PREVIOUS_STABLE: "10.3.X"
separate issue: I think having a mixed usage of tags and dev branches seems a bit strange, I'd suggest to always use -dev, but that's a separate issue? Most importantly for the previous minor, and drop running against the last previous minor *security* release. Then that would becomeCORE_PREVIOUS_MINOR="11.0.x-dev"
And eventually, when 12.x development starts, we can add CORE_MAJOR_DEVELOPMENT: "12.x-dev" or CORE_MAJOR_DEVELOPMENT: "main" and start running next major again.
If we would do an explicit variable for next major, I guess the config would look like this:
CORE_SUPPORTED: "11.1.x-dev"
CORE_SECURITY_PREVIOUS_MINOR: ""
CORE_MAJOR_DEVELOPMENT: "11.x-dev"
_NEXT_MAJOR_NOT_NECESSARY_NOW=1
CORE_NEXT_MINOR: "11.x-dev"
CORE_PREVIOUS_STABLE: "10.3.X"
But as mentioned above, that seems strange to me, because 11.x-dev is not where major development happens, never will be. The introduction of a 12.x/main branach would mean changing two variables.
It is not clear to me how the variables and logic would look like with the not equal check situation. but I guess same, just without the _NEXT_MAJOR_NOT_NECESSARY_NOW? IMHO has the same issue with CORE_MAJOR_DEVELOPMENT just being set to a wrong value for the sake of having a value.
FWIW, my two cents on API policy/stabililty of this project is that this is a CI template not something that runs on production. If breaking a few customized CI configurations allows you to save a considerable chunk of money/energy, then by all means, break stuff. Add some kind of breaking changes section on the documentation that people can find and move on. CI breaks all the time for more complex projects/test setups anyway. pretty much every single new minor version of core caused major test fails in a project like paragraphs. having to update a rule/variable or something in .gitlab-ci.yml sounds like a triviality compared to the kind of debug sessions I had there.
Wim Leers → credited berdir → .
Not caught up on the whole discussion or experience builder storagae in general. I do want to add that the real complexity around translations starts to show when you add pending revisions, aka content_moderation/workflows to the mix. Because that's when you need to start to merge revision data on a per-field or even field-property (in case of ERR/paragraphs, in this case on whatever partial thingies you store/define here).
See also https://berdir.github.io/entities-explained/#/7/4 and slides before and after that.
Mostly an issue for symmetric translations. As an example, assume you have a thing that has a translatable text field, a partially translated image field (file id is synced, alt/title is per-translation) and an untranslatable list/reference field. You might create 3 draft translations in parallel for different languages and at the same time also change the untranslatable list field in the default translation. Then you publish all those in order and the resulting revision should then contain all translations and the untranslatable value from the default translations.
You don't need to and very likely don't want to support that complexity in this initial issue, but it needs to be on the radar, because this is is how content on multilingual sites works (you need to prepare changes on multiple languages and publish them all basically together). Paragraphs/ERR supports this fairly well, getting as far as we are was a large amount of work but people are still struggling with a number of scenarios (for example ✨ Draft translations should be based on the latest revision of the source language, not the published version Needs work and there are several open issues in ERR that I'm struggling to understand).
That's unfortunate, missed that, I'd accept a patch to make it conditional with fallback in a new issue.
berdir → created an issue.
Note: For us, the regular text field was too short as we needed to add a link & markup in the message. For that to also work with translations, we had to change the type from label to text on top of that, so that config translation displays it as a textarea.
Let us know if we should include that in the MR.
I was using an old version (because we were stuck on an old commerce version), looks like that's already improved.
This isn't meant to fix all memory issues. The reports page attempts to provide a summary of possibly GB and millions of keys, it's entirely optional. There is another issue where I suggested a fixed limit or just completely skip parts of it of we detect a too large amount of keys. Profiling and patches welcome.
As usual, there is also at least one duplicate: 📌 Coding Standards Needs review .