- πΊπΈUnited States awm
updated the patch to use filesystemInterface to avoid this error
TypeError: Drupal\publication_date\PublicationDateUpdate::__construct(): Argument #3 ($fileSystem) must be of type Drupal\Core\File\FileSystem, Drupal\s3fs\S3fsFileService given
- π·π΄Romania claudiu.cristea Arad π·π΄
Thank you for the patch. However, creating PHP files on the fly and run them from temp dir it's a terrible idea. Don't do that, it's a security issue.
I am thinking to an approach where the list of entity types is not stored in config but in code, e.g., by exposing a hook whose invocation returns a list of entity types. Then 3rd-party code implements that hook tell which entity types they like to receive the field. Publication date will implement that hook and only return
['node']
. An update hook, will iterate over all entity types and check if the field is installed. If not, will do the proper storage definition install. - Status changed to Needs work
6 months ago 3:39pm 4 June 2024 - π΅πΉPortugal dxvargas
I agree with @claudiu.cristea that creating files on the fly is not a good idea.
Also, I don't see the reason to do this. Can you please explain?
Why not to do the storage updates immediately when saving the config?
I've done this experience and it worked. I can submit a patch.About the config page, I don't see a problem with that.
Using hooks would also work but I actually prefer using the configuration page.I'll reopen the issue for the discussion to continue and the patch to be improved.
- πΊπΈUnited States awm
> creating PHP files on the fly and run them from temp dir it's a terrible idea. Don't do that, it's a security issue.
Agreed...
- π΅πΉPortugal dxvargas
I'm uploading a patch with the approach given by @claudiu.cristea, with a hook whose invocation returns a list of entity types that should have the publication date.
It's far from being a final patch but it works in a basic way and may be useful for next steps. - ππΊHungary huzooka Hungary ππΊπͺπΊ
-
+++ b/publication_date.module @@ -52,17 +52,21 @@ function publication_date_entity_base_field_info(EntityTypeInterface $entity_typ +function publication_date_form_alter(&$form, FormStateInterface $form_state, $form_id) { + $type = isset($form_state->getBuildInfo()['base_form_id']) ? preg_split("/_form$/", $form_state->getBuildInfo()['base_form_id'])[0] : ''; + if (!\Drupal::service('publication_date.enabled_entity_types')->isEnabled($type)) { + return; + } $account = \Drupal::currentUser(); - $node = $form_state->getFormObject()->getEntity(); + $entity = $form_state->getFormObject()->getEntity(); ... }
For first, I would check whether the form object is a
ContentEntityFormInterface
. If so, then we can be sure that theFormstateInterface->getFormObject()->getEntity()
returns a content entity β then we can have the entity type ID without any assumption made on the base form ID:function publication_date_form_alter(&$form, FormStateInterface $form_state): void { $form_object = $form_state->getFormObject(); if (!$form_object instanceof EntityFormInterface) { return; } $entity = $form_object->getEntity(); if (!$entity instanceof FieldableEntityInterface) { return; } $entity_type_id = $entity->getEntityTypeId(); if (!\Drupal::service('publication_date.enabled_entity_types')->isEnabled($entity_type_id)) { return; } ... }
-
+++ b/publication_date.module @@ -52,17 +52,21 @@ function publication_date_entity_base_field_info(EntityTypeInterface $entity_typ - $form['published_at']['#access'] = $account->hasPermission('set any published on date') || $account->hasPermission('set ' . $node->bundle() . ' published on date'); + $form['published_at']['#access'] = $account->hasPermission('set any published on date') || $account->hasPermission('set ' . $entity->bundle() . ' published on date');
These permissions need to be adjusted: e.g. both
node
andtaxonomy_term
entities might have a "foo
" bundle. -
+++ b/publication_date.services.yml @@ -1,5 +1,10 @@ + class: Drupal\publication_date\PublicationDateEnabledTypes + arguments: + - '@module_handler' publication_date.event_subscriber: class: Drupal\publication_date\EventSubscriber\PublicationDateSubscriber + arguments: ['@publication_date.enabled_entity_types'] tags:
Can we standardize the format of the arguments?
Personally, I prefer the format currently used on "
publication_date.enabled_entity_types
" since it is git-friendly. -
+++ b/publication_date.tokens.inc @@ -13,11 +13,15 @@ use Drupal\Core\Render\BubbleableMetadata; + sleep(0);
I guess this is only a debug leftover.
-
+++ b/publication_date.tokens.inc @@ -35,22 +39,22 @@ function publication_date_tokens($type, $tokens, array $data, array $options, Bu /** @var \Drupal\node\NodeInterface $node */ - $node = $data['node']; + $entity = $data[$type];
This variable comment does not make any sense...
Maybe it can be replaced by a condition:
$entity = $data['type']; if (!$entity instanceof FieldableEntityInterface) { return; }
-
+++ b/src/PublicationDateEnabledTypes.php @@ -0,0 +1,62 @@ + + +namespace Drupal\publication_date; + + +use Drupal\Core\Extension\ModuleHandlerInterface;
Nit: Unnecessary empty lines.
-
+++ b/src/PublicationDateEnabledTypes.php @@ -0,0 +1,62 @@ + * Class PublicationDateEnabledTypes + * @package Drupal\publication_date
Niz: Leftover from IDE default s, needs a proper class description.
-
+++ b/src/PublicationDateEnabledTypes.php @@ -0,0 +1,62 @@ + } +}
Nit: missing empty line.
Obviously, we still need some magic to install / uninstall the field whenever it is needed, which (if possible) could be resolved by a ConfigEvent subscriber (save, delete).
-
- π΅πΉPortugal dxvargas
I'm now submitting a new patch with some changes suggested by @huzooka in the previous comment.
- Done
- To do
- Done
- Removed
- Comment removed. I got into a situation where the entity was not an instance of FieldableEntityInterface, for that reason I'm not using your suggestion.
- Done
- Done
- Done