Account created on 17 August 2016, over 8 years ago
#

Merge Requests

More

Recent comments

I suggested it, but I wondered whether moving the class_exists() check in AttributeClassDiscovery::getDefinitions() to go before retrieving the definition from file cache can be avoided. While it does filter out definitions retrieved from cache from being discovered when their modules are not installed, it also means that every plugin class is loaded into memory in the request when the plugin manager gets the definitions from a cold cache. Separately, relying on class_exists to filter out plugins is difficult to test, since all classes are loaded in tests. I also wonder whether there may be a situation during cache rebuilds/module uninstalls where a plugin class in a module that is being uninstalled has already been loaded into memory earlier in the request, so erroneously does not get filtered out.

With this in mind, I tried moving that check back to after the file cache look up, and since Reflection in parseClass() should now be safe, I used reflection to loop through to get all class, interface, and trait dependencies for each plugin class. Then in Core AttributeClassDiscovery, I extracted the modules via namespace of those dependencies and checked whether the modules were installed. This check is done before including the plugin definition in the list returned from getDefinitions().

This broke a lot of tests, because there were usages of node, and entity_test entity types where the user module was not installed, and since those types implement Drupal\user\EntityOwnerInterface, those entity types were not being found without adding 'user' to the installed modules. (Separately there seems to be a lot deprecation warnings for I18nQueryTrait for some weird. reason, though there should not be any classes using the version of that trait in content_translation.)

Regardless, this filtering work probably is out of scope here and maybe belongs in #2786355: Move multiple provider plugin work from migrate into core's plugin system , but I wanted to see if it would work.

1 small nit/comment on the MR.

On a related tangent, has there been thought about deprecating service providers altogether? A lot of what service providers do can be done with service decorators. (Granted that LanguageServiceProvider setting a container parameter is an exception.)

But allowing services to be instantiated in service providers is a blocker to autowiring core services, as seen in 📌 Use autowiring for core modules and services Needs work #48.

I'm not able to reproduce this issue on HEAD with standard profile install:

Steps taken:

  • Install Drupal core with standard profile
  • Uninstall page_cache module
  • Set system.performance cache.page.max to 86400 (Browser and proxy cache maximum age 1 day)
  • Confirm no roles besides administrator have "View user information" (access user profiles) permission
  • Log in as admin and create an Article node and save
  • View the node as anonymous
  • Observe Cache-Control header is "max-age=86400, public" and X-Drupal-Dynamic-Cache is "MISS"
  • Reload and observe X-Drupal-Dynamic-Cache is "HIT"
  • As admin, Add a comment to the node
  • Reload node page as anon and X-Drupal-Dynamic-Cache is "MISS"
  • Reload node page as anon and X-Drupal-Dynamic-Cache is "HIT"

Additionally, in this line in template_preprocess_node():
$variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid']);
$variables['elements']['uid'] contains a render array with the uid field using the "author" formatter (Drupal\user\Plugin\Field\FieldFormatter\AuthorFormatter), which uses the same "view label" entity access check as the label formatter.

I have no opinion on the class name, but made some comments on the MR.

Also:

I feel uneasy about not being able to provide a more specific log message when maintenance mode is set, rather than just logging "The state key system.maintenance_mode has been changed from 0 to 1."

Detecting what the value has changed from isn't possible without get() call, which I presumed was to be avoided. Though I'm also not sure about the performance impact.

I think one (small, probably) concern with not getting the initial stored value in the case of maintenance mode is that if the site was already put into maintenance in a prior request or CLI op, then the log will show successive messages indicating maintenance mode was enabled. This might be confusing to someone expecting to see a message about maintenance mode being disabled somewhere in between.

Maybe the log message for maintenance mode can be worded to acknowledge that maintenance mode state was set but not necessarily changed?

We could add an property to store the keys that are set during the request to \Drupal\Core\State\State and then add a public method to get this information. Then add a service then subscribes to the terminate even and gets this value and logs any state changes for a list of keys defined in a container parameter.

In the case of state, and then perhaps generally, would there be a need to track the previous stored value for the key so it can be compared with the new value?

PHPCS wasn't picking up any white space issues, so I took a guess that it's the deprecation message string.

I also removed the string typehints on getFormName() in (unlikely, granted) the case that there are subclasses overriding the base class method.

I think the IS needs an update, though.

Confirmed that the build jobs are not changed other than ordering. LTGM.

One thing I have discovered is that Drupal\Core\Config\ConfigManager::findConfigEntityDependencies() with the same parameters is not idempotent for the order of the dependencies.

This occurs because when the ConfigDependencyManager is instantiated in getConfigDependencyManager(), the order of config loaded the config factory can vary depending on how much config objects are loaded from the static cache vs database storage. On the first call to ConfigFactory::loadMultiple(), it's possible that only a partial list of config objects were loaded into the static cache, with the remainder needing to be loaded from database storage. On the subsequent calls, all the config objects may already be loaded in the static cache, so the returned order can be different from the first call.

The order the config data is loaded set at ConfigDependencyManager instantiations ultimately affects the sorted order of the dependencies. Trying to fix this by sorting the list of config objects does seem to make findConfigEntityDependencies() idempotent, but this breaks an existing test at Drupal\KernelTests\Core\Config\ConfigDependencyTest::testConfigEntityUninstallComplex().

I'm not sure the results need to be idempotent, either. As long as the order returned by findConfigEntityDependencies() has the correct relative order between dependencies, the overall order probably doesn't need to be consisitent. Meaning, if list of returned dependencies includes a config object (config 1) that depends on another (config 2) within the list, then as long as config 1 comes before config 2 in the list, this should be fine, for all listed dependencies. Investigating further whether true idempotency is required might be out of scope for this issue.

I've added a Kernel test to MR 11141 that creates a similar scenario with permissions being removed incorrectly, that demonstrates how the relative order can be broken per comment #9 when config entities are loaded.

Producing in browser reproduction steps has proven difficult, so the issue mgith be best be observed by the failing test-only job.

If the approach with a new plugin type is used, then this definitely needs an attribute, and probably should replace the annotation.

Though I think @berdir suggested in #102/MR review that tagged services might be a better way to go than plugins?

Investigated this a little, and one thing that's happening is that when the config dependencies are being determined, in the erroneous state, the order of the dependencies returned from Drupal\Core\Config\ConfigManager::findConfigEntityDependencies() looks some thing like this:

  1. default test node entity view display
  2. test role
  3. full test node entity view display

And when it behaves correctly, the order is like this:

  1. test role
  2. default test node entity view display
  3. full test node entity view display

I haven't determined yet why this is happening, but for this specific case, the different ordering should be fine. The test role permission only has a dependency on the full entity display, and not the default one. So when ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval acts on the list of of dependencies, it starts with the end of the list, and as long as the full entity display is processed before the role, everything should be fine.

The problem is that the list from findConfigEntityDependencies() is returned to findConfigEntityDependenciesAsEntities(), where entities are loaded from the list of config names. For efficiency, entities of the same type are loaded in one loadMultiple() operation instead of separate loads, and the order of the list returned becomes:

  1. default test node entity view display
  2. full test node entity view display
  3. test role

With this ordering, the role is processed before the full entity view display and is erroneously handled to remove the permission.

I have put up MR 11141 with a fix for findConfigEntityDependenciesAsEntities(), to make sure the loaded entities stay in the order of the dependency config names. The tests in MR 11120 need to be updated for 11.x, but there were some issues with the config provided by the test modules that needs resolving, then the test can be added to the MR with the fix. Leaving this at Needs Work.

With 🐛 Move content_translation I18nQueryTrait to migrate module Active in, I've rebased MR 6780 and updated it for a couple new additional fixes to get tests passing.

This MR includes converting all existing migrate source plugins from annotations to attributes, though two new test plugins have been added with annotations to test backwards compatibility. In addition, the source_module property is included in the new MigrateSource attribute. There's been discussion that it should not be included because it is strictly for Migrate Drupal, but something like Allow attribute-based plugins to discover supplemental attributes from other modules Active would need to get in for source_module to split from the MigrateSource attribute. Currently that issue does not have much momentum, and I'm not sure this issue should continue to be blocked just to wait on that one.

That being said, MR 6780 also adapts the multiple/automated provider discovery from annotations to attributes, which leads to some tangled pseudo-multiple-hierarchical code. Work in either 📌 Add a fallback classloader that can handle missing traits Active or 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active would help simplify this, but it's possible to move forward here regardless and then re-work once one of those issues gets in.

Regardless, MR 6780 has sat a while, so it's worth looking it over again to see if some things can be improved or simplified. I don't have time to do that right away, so I'm leaving this in NW for now for anyone to pick it up.

Add this as a related issue to 🐛 Form cache causes issues with media library widget Active . Once Firefox testing is in, there should be a test for the form reload behavior documented there.

The changes are needed because the test needs to evaluate the case when the label and value are both string 0, so "0", "1", and "2" need to replace "First", "Second", and "Third".

I created new MR 11081 in case the old test was preferred, but I forgot that the last two commits on MR 10426 weren't on my local.

Anyway, I've closed MR 10426 and decided to go ahead with 11081 so that the last commit diff specifically addresses the MR feedback. I've also rebased to include the two missing commits.

Back to NR.

godotislate changed the visibility of the branch 3258581-move-trait-only to hidden.

I took inspiration from 📌 Add a fallback classloader that can handle missing traits Active and took a look at seeing whether it's possible to set the class loader not to load namespaces for uninstalled modules in a test method. I did this in MR 11081 and was able to create a failing Kernel test. Setting back to NR, since it'll be much faster to use a Kernel test instead of a Functional test.

Can't believe I missed the parameter typehints, but everything should be addressed now.

godotislate changed the visibility of the branch 3502913-test-migrate-source to hidden.

Fixed the namespace and all tests pass except the one in #18. That will be something to address 📌 Convert MigrateSource plugin discovery to attributes Active , but everything should be good for this issue, at least functionally. I think MR 11030 will need another person to review, since I wrote the tests.

The debug classloader being unhelpful again.

Been debugging this, but don't understand what's going on yet.

The other issue is this one, which I don't think is exactly a problem with the changes introduced this issue:

Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest::testGetDefinitions
Failed asserting that an array is empty.

core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php:81

Basically, the Migrate source plugin manager filters out definitions using the ProviderFilterDecorator and the way that AnnotatedClassDiscoveryAutomatedProviders extracts multiple providers for source plugins based on used namespaces. Since the hope here is replace that multiple provider functionality specific to migrate source with the class_exists checks, during test runs plugins are not filtered out as expected because classes/traits/interfaces for all modules are loaded during test runs.

Started MR 11072 incorporating a minimal set of changes from the 📌 Convert MigrateSource plugin discovery to attributes Active MRs to test the problematic source plugins using I18nQueryTrait, on top of the changes from MR 11030.

Have some test failures on that MR will look into later.

Or in Drupal\Component\Discovery?

I think this makes sense, but it's probably fine to leave it in the plugin namespace for now until it's needed for other discovery? Can even use the work from 📌 Add a classloader that can handle class moves Active to move it later. :)

This was mentioned in #11 📌 Allow plugin service wiring via constructor parameter attributes Needs work , but it's been common practice to use ::create() in plugin classes that extend other plugin classes, to avoid issues if the parent constructor's signature changes. I think before deprecating ::create(), at least the suggestion from #12 📌 Allow plugin service wiring via constructor parameter attributes Needs work to accommodate autowired setter injection (presumably with the use of the the \Symfony\Contracts\Service\Attribute\Required atribute) should be done in a follow up.

I made a suggestion that the "class_exists" check be moved to the top, even before trying to retrieve the definition from file cache. This is to skip over definitions that were previously stored in file cache but should be skipped now because modules the definitions have implicit dependencies on are now uninstalled.

IMO, given the relative simplicity of this solution, it's worth going forward on, even with the noted issues. It's better than existing without adding much complexity.

Commented (#27) in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active , where I tested a similar solution. A couple still outstanding points:

  • This relies on traits having names that end in "Trait" (and no classes or interfaces that do), though it might be edge-casey to be using traits that don't
  • Trait use statements that alias methods (and likely similar) will still fatal error. Example:
    use CustomTrait {
        theMethod as theTraitMethod;
    }
    

    Since StubTrait doesn't have "theMethod", reflection still fatal errors.

I gave the suggestions in #25 a shot.

First, I tried registering an autoloader that just throws an exception. That way, all missing classes, interfaces, and traits throw the same exception that can be caught. Unfortunately this does not work for traits. While the exception is thrown, PHP still fatal errors, which looks something like this: PHP Fatal error: During class fetch: Uncaught Drupal\Component\Discovery\DiscoveryException: Plugin class dependency Drupal\a_module_that_does_not_exist\Plugin\CustomTrait is missing. in /var/www/html/core/lib/Drupal/Component/Discovery/DiscoveryClassLoader.php:19

Next, I tried aliasing any missing trait to some placeholder trait. This kind of works, but there are issues:

  • The only way I could figure to do trait detection is to see whether the $class string being passed to the autoloader callback ends in "Trait". While there are only one trait in core that does not end in "Trait" (Drupal\Core\Menu\MenuLinkFieldDefinitions, though there are two other test traits: Drupal\Tests\dblog\Functional\FakeLogEntries, Drupal\Tests\workspaces\Functional\WorkspaceTestUtilities), there's nothing preventing people from creating traits in contrib or custom modules that do not end in Trait. Nor anything preventing people from unfortunately creating classes or interfaces with names that end in Trait, though likely very edge-casey
  • Trait use statements with method aliases still fatal error, because the placeholder trait does not have the method:
    use CustomTrait {
        theMethod as theTraitMethod;
    }
    
  • When the fatal error is avoided by aliasing, the plugin can then be erroneously discoverable. We can get around this by add a constant in the placeholder trait, then using Reflection to detect whether the plugin class has the constant.
    trait PlaceholderTrait {
    
      const PLUGIN_HAS_MISSING_DEPENDENCY = TRUE;
    
      public function __call(string $name, array $arguments) {}
    
    }
    
    // in AttributeClassDiscovery.
    protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
        // @todo Consider performance improvements over using reflection.
        // @see https://www.drupal.org/project/drupal/issues/3395260.
        $reflection_class = new \ReflectionClass($class);
    
        if ($reflection_class->hasConstant('PLUGIN_HAS_MISSING_DEPENDENCY')) {
          // Throw an exception or something.
        }
    

    There would still be an issue in the case where the module containing the trait is installed, so the plugin is discovered. But when the module is uninstalled, the plugin definition would still exist in the file cache, and any unfortunate attempt to instantiate the plugin class would hit a fatal error.

The current test pipelines https://git.drupalcode.org/issue/drupal-3416819/-/pipelines/405923 do seem to have errors still:

That is the "test-only" job. All MR changes outside of test code is removed, and the run is intended to demonstrate that the bug currently exists. The main test jobs passing demonstrate that the changes fix the bug.

The suggested code from review does not handle 0 correctly, so I'm moving the MR as is back to Needs review.

Tremendous weekend work from @catch, and nice find from @longwave.

Will the changes in MR11007 address the similar issues in the test-only job? The configuration in the test-only job uses *with-chrome instead of *with-selenium-chrome, so the Selenium env variable changes probably don't apply?

Or should addressing the test-only job be split into another issue?

godotislate changed the visibility of the branch 3416819-list-field-zero-option-10.3 to hidden.

Oh good catch, went was it passing without it though?

Yes, it's very concerning if functional javascript tests have false negatives on MR builds.

In my MR for another issue: https://git.drupalcode.org/issue/drupal-3416819/-/pipelines/405059/test_...

Also testing locally latest 11.x fails. I have pushed up MR with the fix, but it's concerning that tests were passing earlier.

Read the php-parser docs some, and they recommend reusing the Parser class (https://github.com/Roave/BetterReflection/blob/6.53.x/docs/compatibility...), so I updated the MR to keep the Parser statically after instantiation. I'm not sure what memory or speed impact it has. Running drush si demo_umami -vvv consecutively multiple times did not produce consistent enough metrics to gauge a consistent difference to HEAD.

If this is driven by performance concerns

The performance concern is 📌 Investigate possibilities to parse attributes without reflection Active . This one is about avoiding fatal errors when using reflection to read class attribute data. Leveraging a library here to do that can maybe provide a complete alternative to reflection in the parent issue, but that's not the goal here.

If this is driven by the need not to load the classes, then roave/better-reflection would be IMHO a better solution

I investigated this a little. The current approach here is not to replace Reflection, but to use static analysis (via PhpParserr) to determine whether the class is safe for Reflection, meaning no exceptions or fatal errors because of missing dependencies. Once the class is determined to be safe to reflect, then Reflection is used to get the attribute values. PhpParser works fine for this, and given that BetterReflection is built on top of PHP Parser, it seems unnecessary to use. I did look at whether BetterReflection could be used to replace Reflection in a way that is safe from missing dependencies. Unfortunately, it BetterReflection does not allow creating instances of the attributes (https://github.com/Roave/BetterReflection/blob/6.53.x/docs/compatibility...), so it's not really a fit for this issue.

Added a test. The test works similarly to how trash does: there is a new test module with event subscriber for saving simple config provided by the module. When node and the test module are installed at the same time, the subscriber sets a flag in keyvalue for whether the node entity field table exists at the time of config save.

Refactored this to try to make the functionality available to be used outside plugin discovery.

Rebased and updated the MR:

  • Applied my suggestions for the null checks
  • Removed bundle dependencies when entity permission granularity is at the entity type level
  • The line $this->assertEquals(['entity_test.entity_test_mul_bundle.test'], $permissions['translate test entity_test_mul_with_bundle']['dependencies']['config']); when testing bundle granularity with bundle that is not config seems to be testing the wrong thing, so that has been replaced with $this->assertEquals(['language.content_settings.entity_test_mul.entity_test_mul'], $permissions['translate entity_test_mul entity_test_mul']['dependencies']['config']);
  • Added test to confirm that when bundle translation is disabled, the permission is removed from the role

Is "Needs work" correct as current status, or is this ready to be reviewed again?

@catch Yes, that was part of my thinking of whether there'd need to be something like this for Hook discovery, or as attribute discovery may expand to services or routes, such as 📌 Use PHP attributes for route discovery Needs review , Directory based automatic service creation Needs review , 📌 Support #[AsEventListener] attribute on classes and methods Active , and Support hooks (Hook attribute) in components Active .

@godotislate there is 1 thread I wasn't 100% should be resolved but your comment in #10 made think it maybe could? Could you close it.

It's not my MR, so I can't close it. But the question was resolved.

Changes look good. Hook is no longer an alter hook, and looks like MR comments were addressed.

IS updated.

Are we sure we want to do this. Why did Twig remove the filter?

Per https://github.com/twigphp/Twig/pull/4236

Deprecate the spaceless filter for the following reasons:

  • The performance is bad (as the work is done at runtime via a regexp)
  • Optimizing the size of an HTML doc server side is "almost never" a good idea (compression is better and enough)
  • There are some edge cases where you want to keep some spaces (see https://github.com/twigphp/Twig/issues/3576
  • Controlling whitespace is possible and fine-grained via the dedicated Twig modifiers on {{ }}

If someone find it useful, re-creating it is trivial (return trim(preg_replace('/>\s+<', $content ?? ''));),
but with so many caveats and not so many use cases, I think it does not belong to core.

The rationale for keeping it is that it likely will be very disruptive for a lot of contrib/custom projects, some of which previously had to refactor their templates when Twig deprecated and removed the spaceless tag.

Why do we have both drupal_spaceless and spaceless? Seems unnecessary too introduce drupal_spaceless

Added commit to remove drupal_spaceless.

Noting here that recent test-only runs for the MR on 🐛 Placing a block containing a form in navigation layout prevents layout builder from saving Active having been failing as expected, such as https://git.drupalcode.org/issue/drupal-3493671/-/pipelines/391171. I'm not sure what's different, so it might be an intermittent issue.

A couple suggestions on the MR for missing NULL/empty checks, one of which is causing the majority of test failures.

The update hook is also loading role entities without checking that the role entity type exists, but it's only an issue if the user module isn't installed, which seems like a very unusual edge case.

Updated the log message and restored $this->permissions = $valid_permissions; which had been accidentally deleted. Also added an assertion that the saved role does not have the non-existent permissions.

When making the change, you'd need to make sure the functionality is not affected. If it isn't, then the test needs to be updated as well. There are two test failures, so you'd also need to make sure that the necessary changes can be made and are applied for both.

Addressed MR comments, updated tests, and re-based.

Looks to me that IS was updated, so removing "Needs issue summary update".

This is ready for review.

@arunsahijpal It may be helpful for you to make sure the MR build passes before setting to Needs Review. The build for the latest changes still fails.

See https://git.drupalcode.org/project/drupal/-/merge_requests/10785. You can review the build job log https://git.drupalcode.org/issue/drupal-3497021/-/jobs/3926956 to see why it is failing.

Looks like everything was addressed. PHP Unit Build job needs a re-run and we're probably there.

Resolved merge conflict and rebased. As mentioned in #13, core has test coverage to make sure spaces and special characters in source names work (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php?ref_type=heads#L644), so I don't think it's necessary to duplicate here.

Moving to NR.

In general, I'm unsure about the Class suffixes. I think I'd prefer that thy all use handler somehow, so for example AccessHandler, not AccessClass.

Changed all the handlers from something like AccessClass to AccessHandler. Given that...

I think it's definitely worth adding a RouteProvider attribute as well, that greatly simplifies the syntax: #[RouteProvider('html', NodeRouteProvider::class)]

Added this as RouteProviderHandler, since now all the other handlers have that suffix. I do not feel strongly about this, though.

I also added a EntityKey (which I guess could just be "Key") and a Label attribute, because they are commonly used enough. Adding Label also provides headway for #2813895: Combine entity type labels in an array , or at least, a start to a way to provide new types of labels more easily. (There'd still need to be an API change on how to get those labels.)

Along those lines, here are some pros to splitting up the entity type attributes like this:

  • It is more schema-agnostic for developers working on their own entity type definitions (maybe somewhat mitigated by devs using drush generate)
  • Can set arbitrary properties on entity type definitions (though entity types already have $additional)
  • Typehinting for property values otherwise within nested arrays
  • Each attribute can provide more documentation about its property, though the classes are currently only lightly documented since this is POC

For cons:

  • As mentioned in other comments, it's not necessarily more readable
  • It's more typing, unless an IDE helps out with that via tab completion or similar
  • Conversion to use them on existing entity types would be a large effort (though this maybe this is optional?)

FWIW, making sure the form_build_id hidden input value does not get retained by the browser was enough to fix the issue for me on Firefox. I put up a draft MR 10706 with that change.

Per conversation with catch on Slack, the overall goal is to change everything in tests over to key value, except for tests specifically testing state, because writing to key value will be cheaper than writing to state.

The references changed in the MR were ones specific to LayoutBuilderBlocksTest and everything that shared those references. I think it's just a question of how to scope changing over all the state usage across all the tests.

@bkosborne What browser did you test in?

For me testing locally on latest 11.x dev:

  1. drush si standard -y
  2. drush en media_library
  3. Add image media field to basic page
  4. Create basic page, upload media image, save
  5. Edit basic page
  6. Remove selected media

On Windows, Chrome 131.0.6778.205, when I reload, the media selection is restored on the page. When I click the remove button again, the media item is correctly removed.

Also on Windows, Firefox 133.0.3, I can reproduce the issue mentioned in the IS. However, if I revisit the page (while on the page, click in location bar and hit enter) instead of reloading, then the remove button works as expected.

It seems to be related to Firefox keeping form data on reload: https://stackoverflow.com/questions/7377301/firefox-keeps-form-data-on-r.... It's an old issue on stackoverflow, but I can confirm that if I add the autocomplete="off" attribute to the node form, e.g. $form['#attributes']['autocomplete'] = 'off';, then reloading on Firefox works the same as Chrome.

Per discussion on Slack, we can go forward with the state to keyvalue swap for the relevant block tests here. There should be a follow up though to investigate further about what is the cause of the intermittent failures state failures, if only to establish that it's a test-only condition.

The other random test failure issues linked in the meta 🌱 [meta] Known intermittent, random, and environment-specific test failures Active should be looked at as well to see whether there are usages of state that can be replaced with key value instead.

godotislate changed the visibility of the branch 3477586-2500x-LayoutBuilderBlocksTest-testBlockPlaceholder-as-is to hidden.

godotislate changed the visibility of the branch 3477586-2500x-LayoutBuilderBlocksTest-testBlockPlaceholder-replaced to hidden.

Tried re-running the test-only job, and it failed as expected! https://git.drupalcode.org/issue/drupal-3493671/-/jobs/3825109.

I'm not sure what's differe, but unpostponing and putting back to NR.

Thanks for the review, @plopesc. I made the suggested change as well as a similar one I forgot.

Not sure what's going on with the test. I created 🐛 Functional Javascript test false postive and missing browser output Active , because I have observed some unexpected behavior going on with the browser-based tests at least, but I've made little headway in identifying the cause. Postponing this issue on the resolving the tests.

One thing I have noticed is that on test-only job, this message shows twice: HTML output directory sites/simpletest/browser_output is not a writable directory.

Example: https://git.drupalcode.org/issue/drupal-3493671/-/jobs/3819911

Pushed up draft MR 10689 with just the state -> keyvalue changes mentioned in #10. Only can report that the the build did not fail.

Refactored so that an event subscriber isn't necessary. #pre_render and #post_render callbacks can handle it. Tests pass, but still not getting the test only build to fail as expected in CI, though.

The only BC safe way would be some func_get_args() trickery.

I think this might make sense, along with a deprecation triggered. Is there also a way to annotate in the interface that the method parameters will change?

I'm not keen on special-casing some handler types over others. I'm not sure why EntityTypeInterface does that anyway!

I think someone in the other issue mentioned there'd be plenty of bikeshedding over which attributes to create and what they should be named, so might as well start now :).

I'm not that opinionated about this, though I will say it's nice to align with the set*() methods in that it leverages API as opposed to making assumptions about the definition schema. (The HandlerClass attribute is special cased because setHandlerClass doesn't deal with nested hander definitions.) On the other hand, with adequate test coverage, the point is largely moot.

Separately, I don't love how long "EntityTypeProperty" is, but "Property" on its own is useless as a search string.

With the attributes it's just one massive block of text.

I don't know if it looks better this way, either. Though it does look better in an IDE (or at least PHPStorm) than it does in Gitlab UI. And the attributes that have multiple arguments can be made to be multiline if preferred.

Changing the signature to get() to have two arguments or adding a "safe" method to FieldableEntityInterface would both introduce a BC break. I think it'd make more sense to do the former, if for no other reason that it doesn't require an additional change to the Twig allow list. (Though I imagine most Twig templates use the magic getter instead of get).

Production build 0.71.5 2024